kernel: leasablebuffer: add APIs #3505
Conversation
3e77aee to
7abb770
Compare
| let new_start = self.active_range.start.checked_sub(left).ok_or(())?; | ||
|
|
||
| self.active_range = Range { | ||
| start: new_start, |
There was a problem hiding this comment.
This seems dangerous doesn't it?
Isn't the point of LeasableBuffer that we can pass a portion of the buffer to lower levels and trust that they aren't changing any data outside of the active range. This seems to encourage lower levels to change data outside of the range.
What if a lower level calls this but the upper level doesn't expect it to? Won't it then override data?
There was a problem hiding this comment.
What if it calls take() (like siphash did)? Or reset()?
You can see my use case here: https://github.com/tock/tock/pull/3508/files#diff-d4c838ce4fe52e989c391df3c23c81855d0eb3396656f4ca281a609a89c02391R215
This saves us from having to move bytes in the buffer to make room for a header.
There was a problem hiding this comment.
I agree that currently nothing enforces it. I thought the idea with take() is that the lower layers still used the length (but that would break with a non zero start offset). reset() I thought was never to be called by lower layers.
I understand that it avoids copying data within the buffer, but it seems like a risky function to implement as then all lower layers can use it and there isn't a way to determine if we should or shouldn't safely call it
There was a problem hiding this comment.
Why is it more risky than expecting that the provided buffer has room to the right?
Either we unslice to the left, or move bytes to the right then fill in the vacated space. Either way we make assumptions about the buffer passed in. If there isn't room we return an error in either case.
But, I'm arguing that this is the API we want. Rather than implicitly expecting buffers to be large, we explicitly direct callers to leave space so we don't have to move bytes around.
There was a problem hiding this comment.
But we know the length of the buffer. When a lower layer gets passed a LeasableBuffer it has a start and an end offset. So we know for sure that when we move data it is inside that buffer, there aren't any assumptions.
How do we direct callers to leave space at the start? Just a comment in the function that we hope they read? That seems not a very Rust-y way to do this
There was a problem hiding this comment.
But, if we do want to preserve the ability to make LB a protection mechanism, what would it look like to expand the type somehow to support both uses? Could we add an API to allow the creator to specify that extending/unslicing is ok? Or is that compromising the usability of the type?
There was a problem hiding this comment.
IMO LB is a convenience mechanism to emulate slices, that we are forced to use because no_heap Rust requires us to reuse static buffers, and static lifetimes are not compatible with passing slices. If we could pass slices of a static buffer and then reassemble the static buffer after a callback LB would not exist. So I think LB should try to emulate the slice API as closely as possible, for both the convenience and protection that provides, except where we are forced to make concessions so that it is usable in practice.
I think if we design LB such that people are encouraged to use it differently than they would a slice that will lead to programming mistakes due to mismatched expectations. Maybe I am wrong though, as you and Amit clearly do not see LB as a slice replacement as much as I do
There was a problem hiding this comment.
After thinking about this for a bit, I'm on Hudson's side here. I think it's not really about protection, but rather expectations. If a higher level passes down a LB that is limited to some region, why should a lower level be expanding that? If the higher level wanted the lower level to use that space, it could have included it in the LB range to begin with. It seems to me that each layer should use either the space given to it, or some subset of that space, but not more.
There was a problem hiding this comment.
I don't think SubSlice can be an enforced protection mechanism; once you move to an asynchronous model, you can't be assured of a linear call/return chain that allows you to telescope out of reducing access scopes. Put another way, it can only work as a protection mechanism if you assume that SubSlices always return to their callers (which they might not), and that would require storing some state for each caller. Put another way, the bottom of the call chain does in the end need access to the complete buffer because they might need to pass it off to someone else.
That being said, following Hudson's point, it is intended that they are often used kind of like slices. They're a way to easily pass the state of a sub-slice down. So making them look like slices as much as possible is good, as long as we don't break assumptions/lead to confused use.
There was a problem hiding this comment.
While I don't agree that SubSlice should emulate what LB would look like if Rust gave us a native way to do what we want (as Hudson explained well), I do understand that mimicking what a rust developer would reasonably expect a "sub" slice type to behave like has quite a bit of value.
If I'm extending this discussion just slightly, what I'm hearing is that if it is going to be called SubSlice (or some other riff on "slice") then it should act like a normal slice to the extent possible (and we just need to tell people not to call take). It's not that we expect to enforce this with the compiler, just that we want to match developer expectations.
But, I'm also hearing that it's not that we shouldn't provide a type that exposes unslice or extend, it's just that would be a different type, with a name that reflects its purpose, if it were to exist.
| /// This returns `Ok(())` if there is room to increase the range the | ||
| /// requested number of elements to the left. This returns `Err(())` if | ||
| /// there is not enough room, and the accessible range remains unchanged. | ||
| pub fn unslice_left(&mut self, left: usize) -> Result<(), ()> { |
There was a problem hiding this comment.
What about a more general extend(&mut self, left: usize, right: usize) -> Result<(), ()>.
Probably not what we want in this type.
|
I removed |
Pull Request Overview
This pull request updates
LeasableBufferto add:as_slice(): gets a sliceis_sliced(): whether .slice has been calledunslice_left(): open up room to the leftTesting Strategy
Updating k-v stack.
TODO or Help Wanted
This doesn't have to be the last PR that adds APIs, but I would like to not stall on this because these are the changes needed to update the K-V stack.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.