Re: [PATCH V4 4/8] libgpiod: Add rust wrapper crate

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 27-07-22, 10:57, Kent Gibson wrote:
> On Fri, Jul 08, 2022 at 05:04:57PM +0530, Viresh Kumar wrote:

> > +    /// Get the path used to find the chip.
> > +    pub fn get_path(&self) -> Result<&str> {
> 
> It seems absurd that a method that simply returns the path provided to
> open() requires a Result, but that is a consequence of wrapping.
> 
> I was considering suggesting caching a copy in struct Chip, but all the
> other methods require Results so at least this is consistent :-(.
> 
> Yay, more unwrapping than Xmas past.

:)

> > +        // SAFETY: The string returned by libgpiod is guaranteed to live as long
> > +        // as the `struct Chip`.
> > +        let path = unsafe { bindings::gpiod_chip_get_path(self.ichip.chip()) };
> 
> Trusting that it is never NULL?

I believe we discussed that early on (few months back) and decided this will
never be NULL and since the Rust wrappers are pretty much part of the same
repository, we can take that as a guarantee. An out-of-this-repository user
can't really assume that.

> Add a null check to be sure.

But I am fine with this as well.

> > +    /// Wait for line status events on any of the watched lines on the chip.
> > +    pub fn wait_info_event(&self, timeout: Duration) -> Result<()> {
> 
> Durations cannot be negative, so caller cannot make this block
> indefinitely.  Make timeout an Option? (no timeout => block)

I didn't know we want the always blocking capability as well. Yeah, Option
sounds like the right approach.

So in that case we just pass -1 to gpiod_chip_wait_info_event() ?

> > +        let ret = unsafe {
> > +            bindings::gpiod_chip_wait_info_event(self.ichip.chip(), timeout.as_nanos() as i64)
> > +        };

> > diff --git a/bindings/rust/src/chip_info.rs b/bindings/rust/src/chip_info.rs
> > +/// GPIO chip Information
> > +pub struct ChipInfo {
> > +    info: *mut bindings::gpiod_chip_info,
> > +}
> > +
> 
> Consider modules and namespaces rather than lumping everything in
> the gpiod space.
> 
> e.g. gpiod::ChipInfo -> gpiod::chip::Info

The modules are already there, as file names. So what we really have is
gpiod::chip_info::ChipInfo (yeah it isn't great for sure). But then it looked
tougher/complex/unnecessary for end users to know the internals of a dependency
and so I did this:

pub use crate::chip::*;
pub use crate::edge_event::*;
pub use crate::event_buffer::*;
pub use crate::info_event::*;
pub use crate::line_config::*;
pub use crate::line_info::*;
pub use crate::line_request::*;
pub use crate::request_config::*;

which puts everything under gpiod::*. I think it is easier for users that way.
The modules are fine for in-crate management, but for end user they shouldn't be
visible.

> > diff --git a/bindings/rust/src/lib.rs b/bindings/rust/src/lib.rs
> > +/// Error codes for libgpiod operations
> > +#[derive(Copy, Clone, Debug, PartialEq, ThisError)]
> > +pub enum Error {

> > +    #[error("Operation {0} Failed: {1}")]
> > +    OperationFailed(&'static str, IoError),
> 
> Use an enum for the operation rather than a string?

Not sure I understood this.

> And if it returns an IoError it must be an IoOperation?
> Else if the IoError is just an errno then call it that.
> 
> > +}

> > diff --git a/bindings/rust/src/line_info.rs b/bindings/rust/src/line_info.rs
> > +/// Line info
> > +///
> > +/// Exposes functions for retrieving kernel information about both requested and
> > +/// free lines.  Line info object contains an immutable snapshot of a line's status.
> > +///
> > +/// The line info contains all the publicly available information about a
> > +/// line, which does not include the line value.  The line must be requested
> > +/// to access the line value.
> > +
> > +pub struct LineInfo {
> > +    info: *mut bindings::gpiod_line_info,
> > +    ichip: Option<Arc<ChipInternal>>,
> > +    free: bool,
> 
> This flag makes no sense - the info always needs to be freed no matter
> which path, watched or not, was taken to get it from the C API.

Not the one created with gpiod_info_event_get_line_info(), else it will result
in double free.

> > +    /// Stop watching the line
> > +    pub fn unwatch(&mut self) {
> > +        if let Some(ichip) = &self.ichip {
> > +            unsafe {
> > +                bindings::gpiod_chip_unwatch_line_info(ichip.chip(), self.get_offset());
> > +            }
> > +            self.ichip = None;
> > +        }
> > +    }
> > +
> 
> This should be a method of the chip, not the info.

I think there were some issues with my understanding of the whole watch thing.
Is there any existing example of gpiowatch somewhere ?

> > +impl TryFrom<&InfoEvent> for LineInfo {
> 
> Is try_from appropriate for getting a contained object?
> "from" normally refers to a type conversion.

Not sure, but as most of new() is going away here, I will likely move this to
info_event as well. No try-from with that.

> > +impl Drop for LineInfo {
> > +    fn drop(&mut self) {
> > +        // We must not free the Line info object created from `struct InfoEvent` by calling
> > +        // libgpiod API.
> > +        if self.free {
> > +            self.unwatch();
> 
> Why does dropping a LineInfo unwatch the line???

Because I thought it is related to the Line's info and we won't wanna watch once
line info is gone. Again, it is my wrong interpretation.

> > +    /// Get values of a subset of lines associated with the request.
> > +    pub fn get_values_subset(&self, offsets: &[u32], values: &mut Vec<i32>) -> Result<()> {
> > +        if offsets.len() != values.len() {
> > +            return Err(Error::OperationFailed(
> > +                "Gpio LineRequest array size mismatch",
> > +                IoError::new(EINVAL),
> > +            ));
> > +        }
> > +
> 
> Returned values are awkward to use as the user has to index into them
> using the index corresponding to the offset in offsets.
> Provide a Values type that maps offset to value, e.g. using an IntMap,
> and pass that in instead of separate offsets and values arrays.

We would need to separate out the offsets then as that's what
gpiod_line_request_get_values_subset() needs. Maybe take offsets, like now, as
an argument and return Result<IntMap> ?

> Same applies to set_values_subset().

That would be fine here though.

> > +    /// Set the size of the kernel event buffer for the request.
> > +    ///
> > +    /// The kernel may adjust the value if it's too high. If set to 0, the
> > +    /// default value will be used.
> > +    pub fn set_event_buffer_size(&self, size: u32) {
> > +        unsafe {
> > +            bindings::gpiod_request_config_set_event_buffer_size(self.config, size as c_ulong)
> > +        }
> > +    }
> 
> The kernel may adjust the value regardless - this value is a tentative
> suggestion to the kernel (the kernel buffers have to be sized in 2^N, so
> it generally rounds up to the next power of 2, within limits).
> 
> > +
> > +    /// Get the edge event buffer size for the request config.
> > +    pub fn get_event_buffer_size(&self) -> u32 {
> > +        unsafe { bindings::gpiod_request_config_get_event_buffer_size(self.config) as u32 }
> > +    }
> 
> You might want to note that this just reads the value from the config.
> The actual value used by the kernel is not made available to user space.

Do you want me to add these two comments for the above two routines ?

Other comments, which I removed, look acceptable to me. Will try to incorporate
all that in the next version.

Thanks a lot for the very thorough review.

-- 
viresh