Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: add StorageIterate #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

codyps
Copy link

@codyps codyps commented Apr 11, 2023

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: esp-rs/esp-idf-svc#249

@ivmarkov
Copy link
Collaborator

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?

@ivmarkov
Copy link
Collaborator

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

@codyps
Copy link
Author

codyps commented May 10, 2023

@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.

This allows iteration over entries within a a storage instance.
@codyps
Copy link
Author

codyps commented May 11, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants