embedded-svc

embedded-svc copied to clipboard

StorageIterate provides a way to iterate over all the keys within a Storage instance.

Some notes:

• This borrows the storage instance while the iterator exists, even though this might not be needed by all implementations
• This returns Result<> both on the initial entries() call and on each step. This mirrors https://doc.rust-lang.org/std/fs/fn.read_dir.html
• I have no particular attachment to the name used.
• Having the StorageIterate::Error type parameter makes things more flexible when layering over a StorageImpl compared to inheriting from StorageBase.
• I'm not entirely happy with the name() and name_cstr() functions on each entry. They mainly exist because on esp-idf's nvs we have cstrings as the underlying data, and I did not want to prevent obtaining entries with non-utf-8 names. In practice, it might be ok to skip non-utf-8 entries instead of allowing access to the CStr.

---

esp-idf-svc PR: https://github.com/esp-rs/esp-idf-svc/pull/249

Two notes:

• C_str is an ``ESP-IDF-ism, so we should avoid it here (embedded-svc does not assume there is a C implementation underneath), as this is an API leak.
• More importantly - I'm unsure why we need the StorageIterate trait (btw - shouldn't it be named StorageIterable)? We already have IntoIterator in Rust core, and if you want to want an "iterable" behavior you just need to implement IntoIterator with Item = &'a str for &'a T where T: Storage?

@jmesmon Do you still plan to work on this and address my comments?

@ivmarkov ya, sorry about not getting back to you on this. I agree about the CStr bit (it should be removed), and seeing if IntoIterator might work here sounds like a good idea.

I'm not sure when I'll take a look though. Might be soon or might not be.

I pushed a change removing the CStr bits which seems to be fine.

I looked at using IntoIterator, but I think we'll have some trouble with that:

• IntoIterator doesn't allow us to have a Result from the initial iterator generation (ie: it isn't TryIntoIterator). We could fold the error into the first result from iteration where needed (though that is a bit awkward), so this might not be fatal. On "should getting the iterator return a result", std::fs::read_dir does, and so do the nvs C apis.
• The lifetimes on the keys make things tricky. We get a blob of owned data that contains the key, and need to hand out a reference to that data. If our iterator returns &str, we need to store the blob of owned data within the iterator itself, so the lifetime of the name: &'a str needs to be the lifetime of the iterator. But Iterator doesn't allow that. One can do it with generic associated types though (https://rust-lang.github.io/rfcs/1598-generic_associated_types.html), but if we're going to do something custom, then returning the actual object that owns the name (Entry) makes things a bit more usable for users and avoids more complicated lifetimes.

I might have missed a way to go about this with Iterator<Item = &str> / IntoIterator though.