Allow apps using the bridge to respond to powerbox requests & offers #3222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ok, I think the documentation part of this is ready-ish. I'm sure there are improvements to be made, but I think the proposed interface is at least described now. |
ocdtrekkie
reviewed
Feb 15, 2020
zenhack
changed the title
|
OK! the version of the code here is "done;" I'd like to add a couple automated tests, but those will probably cause some annoying conflicts with #3221, whereas the rest of these two prs merges cleanly. I've got the filesystem demo working with this well enough to have some confidence though (there are a few bugs in the demo that are clearly not related to the bridge). I consider this ready for review. |
kentonv
reviewed
Feb 22, 2020
The next few commits will flesh out relevant documentation to mention the header described in the comments, and describe how to use these methods. All this will serve as a more precise spec for what to implement.
Still need to talk about offer sessions.
Not tested (though it doesn't break the existing tests).
Thanks to @ocdtrekkie for spotting these.
We're going to start tweaking this to support offer & request sessions, so now is a good time to do this cleanup.
This needs further testing, but is now feature complete wrt. sandstorm-io#3197
It now doesn't crash until those methods are called, rather than on creation. I'm going to put this down for a bit and come back when I have the mental energy to sit down and understand what's going on.
Apparently I messed this up when factoring out the logic.
...mostly in place of kj::mv. At least one of these was responsible for some memory corruption I was seeing. Some of the copies this introduces *may* be unnecessary, but it's probably better to have the copy in there than the unsafe cast, until it is demonstrably a problem.
See the comments for the current state of affairs. The code I'd written previously didn't have any coherent story here, and was *sometimes* working I think just because of luck wrt. what memory gets re-used by the allocator or doesn't. This needs more testing, but experimenting with my filesystem demo, grains, it gets further than anything before it.
|
Merged "cleanly", but there are some build errors now. Investigating. |
kentonv
reviewed
Feb 22, 2020
...instead of three, per @kentonv's suggestion.
kentonv
reviewed
Feb 22, 2020
Readers are already reference types, so let's just store that directly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Starting with just spec'ing out the solution I described in #3197 precisely; I've done the capnproto parts of this, and will push subsequent commits that update other relevant parts of the docs. Once that's in place and folks are happy with the API, I'll start implementing. I'll probably wait until #3221 is merged and rebase on top of that to avoid potential conflicts.