search

fpagliughi / rust-industrial-io

Rust interface to the Linux Industrial I/O subsystem
MIT License
45 stars 21 forks source link

Some memory safety fixes #3

Closed skrap closed 5 years ago

skrap commented 5 years ago

Hello! I have been using this crate for some prototype work. Thanks! I ran into issues with runtime crashes due to Context structs being dropped while there were still Device and Channel structs alive which (internally within libiio) still referred to the iio_context. I took some time to do some internal refactoring to add reference counting so that this crash would be impossible. I think it slightly improves the ergonomics as well, as the user need not keep around a Context object if they don't want it. I removed some APIs which were not immediately easy to port to this new scheme. See the commit comments for more info. I hope this is useful!

fpagliughi commented 5 years ago

Wow. Thanks. As you probably noticed, I've left this project sitting for a few months, but coincidentally, I am about to start working on it again to get a production-quality release in the next few weeks. I will look at this shortly.

fpagliughi commented 5 years ago

Sorry for the long delay. I'm finally getting back to this project.

The updates look good, and are definitely necessary. I am worried about the breaking change to the meaning of Context::clone(), however. The iio_context_clone() call creates a copy of the context that is safe to use from another thread.

In Rust Embedded Linux crates, it seems that Clone is commonly used to increment the reference count to the same object rather than making a deep copy. But I believe that it's also quite common to use clone to make a copy of a thing that can then be sent to another thread - either via Arc or deep copy.

Due to the library call, an existing IIO user might have an opinion on the meaning of cloning a context that assumes creating a copy in the library which is save to use in another thread.

So we have two competing ideas of what "clone" means, and I see them as equally valid. And we should implement both. But how do we do that without being totally confusing?

skrap commented 5 years ago

Good question... I think clone is more fundamental to Rust than it is to IIO. (I never even knew cloning a IIO context was something which could be done, and I've been using IIO for years!) However, it's nearly impossible to do anything of consequence in Rust without using clone(). So I'd favor clone staying with its Rusty meaning, and clone_inner or clone_context or maybe iio_clone to be the name of the callthrough to the underlying library. If the name was clone_context then someone autocompleting with RLS or looking through the docs would see both listed next to each other, and would hopefully be curious enough to disambiguate them.

fpagliughi commented 5 years ago

OK. Nevermind. If the Context is not Send, then we can't clone it using the library and then move it into another thread anyway. It would be a useless trap! I suppose a Context could only be Send if it is newly-constructed and hasn't been used to create a Device yet. But I don't know how to police that.

So we'll go with the "rustier" definition of Clone and increment the ref count.

fpagliughi commented 5 years ago

I cherry-picked out the rustfmt commit, as it's clashing with a local branch I haven't pushed yet. At some point, I'll format and push. I also renamed RawContext as InnerContext as that seems more of a convention I've seen in other crates. Embedded Linux ones, anyway.

Thanks again! I hope to have a new release ready targeting v0.18 in a few weeks, possibly doing a better job at generating auto-building the C lib, etc.

skrap commented 5 years ago

Awesome! I left the rustfmt in a separate commit for just this reason. Glad it worked out! And thanks for your work on this crate. Very useful! :balloon: :tada: