capsules: tickv: call error cb on append error #3489
Conversation
capsules/extra/src/tickv.rs
Outdated
| match ret { | ||
| Ok(tickv::success_codes::SuccessCode::Complete) | ||
| | Ok(tickv::success_codes::SuccessCode::Written) | ||
| | Err(tickv::error_codes::ErrorCode::BufferTooSmall(_)) => { |
There was a problem hiding this comment.
Why do we signal this as a success to the caller?
There was a problem hiding this comment.
Because on delete our kv store layer only reads the header, not the entire value, so it only provides a smaller buffer.
There was a problem hiding this comment.
I don't follow. What delete?
There was a problem hiding this comment.
kv_store::delete().
To remove an object we call get with a buffer large enough to read just the tock tickv header.
There was a problem hiding this comment.
I still don't follow.
So in kv_store the generate_key_complete() function calls get_value() with a hashed key and a buffer to store the value.
That ends up triggering the read_complete() callback for a Operation::GetKey in tickv.rs.
The ret is an Err(tickv::error_codes::ErrorCode::BufferTooSmall(_)) which triggers the get_value_complete() callback.
But then get_value_complete() doesn't check result because it expects a failure. So what doesn't work? The PR and commit messages don't really explain what this is fixing
There was a problem hiding this comment.
How can kv_store not check result? What if some other error occurs?
There was a problem hiding this comment.
Yeah, I agree it should check result.
A small change like this:
diff --git a/capsules/extra/src/tickv.rs b/capsules/extra/src/tickv.rs
index d70e4e13a..fa91f5319 100644
--- a/capsules/extra/src/tickv.rs
+++ b/capsules/extra/src/tickv.rs
@@ -282,6 +282,16 @@ impl<'a, F: Flash, H: Hasher<'a, 8>, const PAGE_SIZE: usize> flash::Client<F>
);
});
}
+ Err(tickv::error_codes::ErrorCode::BufferTooSmall(_)) => {
+ self.operation.set(Operation::None);
+ self.client.map(|cb| {
+ cb.get_value_complete(
+ Err(ErrorCode::SIZE),
+ self.key_buffer.take().unwrap(),
+ self.ret_buffer.take().unwrap(),
+ );
+ });
+ }
Err(tickv::error_codes::ErrorCode::EraseNotReady(_)) | Ok(_) => {}
_ => {
self.operation.set(Operation::None);and then get_value_complete() can check the result and ignore the ErrorCode::SIZE as expected
There was a problem hiding this comment.
Can you create a branch or suggested change?
There was a problem hiding this comment.
Something like this? alistair23@7671497
| @@ -45,7 +45,7 @@ use kernel::utilities::leasable_buffer::LeasableMutableBuffer; | |||
| use kernel::ErrorCode; | |||
| use tickv::{self, AsyncTicKV}; | |||
|
|
|||
| #[derive(Clone, Copy, PartialEq)] | |||
| #[derive(Clone, Copy, PartialEq, Debug)] | |||
There was a problem hiding this comment.
Why do we need this to be Debug?
There was a problem hiding this comment.
I've done a lot of debugging.
There was a problem hiding this comment.
Ha!
But it does add code size and it isn't being used by default
There was a problem hiding this comment.
I don't think it should add size in the case that it isn't being called
capsules/extra/src/tickv.rs
Outdated
| match ret { | ||
| Ok(tickv::success_codes::SuccessCode::Complete) | ||
| | Ok(tickv::success_codes::SuccessCode::Written) | ||
| | Err(tickv::error_codes::ErrorCode::BufferTooSmall(_)) => { |
There was a problem hiding this comment.
I don't follow. What delete?
|
Added |
| match ret { | ||
| Ok(tickv::success_codes::SuccessCode::Complete) | ||
| | Ok(tickv::success_codes::SuccessCode::Written) => { | ||
| self.operation.set(Operation::None); |
There was a problem hiding this comment.
Appending doesn't need a callback?
There was a problem hiding this comment.
Right. So appending is read (check if the key exists) then a write (write the new key-value) so the callback happens after the write in the write_done callback.
| buf_buffer.map(|buf| { | ||
| self.ret_buffer.replace(buf); | ||
| }); |
There was a problem hiding this comment.
Generally, the system of continue_operation() taking a buffer and then returning it, then this code needing to put that buffer right back, but you've got to make sure you put it in the right place, seems like a bug waiting to happen.
I'm satisfied that you did it correctly for GetKey and AppendKey (I think). And I'm not saying you should rewrite it now, but to me it feels bad.
There was a problem hiding this comment.
I think this gets better with the full set of changes. I tried to clarify that there are key buffers and value buffers so it's easier to keep track of them and where they need to go.
Pull Request Overview
In the tickv capsule we need to signal a callback even if the append fails.
Testing Strategy
Writing to an existing key.
TODO or Help Wanted
n/a
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.