Skip to content
New issue

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

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

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

3 participants