Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> For Rust changes, please run these as well to find any formatting
> issues, warnings:
>
> cargo fmt --all -- --check
> cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -D clippy::undocumented_unsafe_blocks
>
> On 13-01-23, 22:51, Bartosz Golaszewski wrote:
> > diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
> > index 19dc187..0c8b293 100644
> > --- a/bindings/rust/libgpiod/src/line_config.rs
> > +++ b/bindings/rust/libgpiod/src/line_config.rs
> > @@ -2,8 +2,8 @@
> >  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> >  // SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> >
> > -use std::os::raw::{c_ulong, c_void};
> > -use std::slice;
> > +use std::os::raw::c_ulong;
> > +use std::collections::HashMap;
> >
> >  use super::{
> >      gpiod,
> > @@ -77,51 +77,32 @@ impl Config {
> >          }
> >      }
> >
> > -    /// Get line settings for offset.
> > -    pub fn line_settings(&self, offset: Offset) -> Result<Settings> {
> > -        // SAFETY: `gpiod_line_config` is guaranteed to be valid here.
> > -        let settings = unsafe { gpiod::gpiod_line_config_get_line_settings(self.config, offset) };
> > -
> > -        if settings.is_null() {
> > -            return Err(Error::OperationFailed(
> > -                OperationType::LineConfigGetSettings,
> > -                errno::errno(),
> > -            ));
> > +    /// Get a mapping of offsets to line settings stored by this object.
> > +    pub fn line_settings(&self) -> Result<HashMap<Offset, Settings>> {
>
> Just like ValueMap, maybe we can add following in lib.rs for this:
>
> pub type line::SettingsMap = IntMap<line::Settings>;
>
> > +        let mut map: HashMap<Offset, Settings> = HashMap::new();
> > +        let num_lines = unsafe { gpiod::gpiod_line_config_get_num_configured_offsets(self.config) };
>
> This needs a SAFETY comment. Should we check if this returned 0 ?
>
> > +        let mut offsets = vec![0; num_lines as usize];
> > +
> > +        // SAFETY: gpiod_line_config is guaranteed to be valid here.
> > +        unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config,
> > +                                                                 offsets.as_mut_ptr(),
> > +                                                                 num_lines) };
>
> Can the returned value be < num_lines here ?
>

Ah, of course it can. Need to add a test case for that. How do I set
the size of offsets to whatever this function returns?

> > +
> > +        for offset in offsets {
> > +            // SAFETY: `gpiod_line_config` is guaranteed to be valid here.
> > +            let settings = unsafe { gpiod::gpiod_line_config_get_line_settings(self.config,
> > +                                                                               offset) };
> > +            if settings.is_null() {
> > +                return Err(Error::OperationFailed(
> > +                    OperationType::LineConfigGetSettings,
> > +                    errno::errno(),
> > +                ));
> > +            }
> > +
> > +            map.insert(offset, Settings::new_with_settings(settings));
> >          }
> >
> > -        Ok(Settings::new_with_settings(settings))
> > -    }
> > -
> > -    /// Get configured offsets.
> > -    pub fn offsets(&self) -> Result<Vec<Offset>> {
> > -        let mut num: u64 = 0;
> > -        let mut ptr: *mut Offset = std::ptr::null_mut();
> > -
> > -        // SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long
> > -        // as it is not explicitly freed with `free()`.
> > -        let ret = unsafe {
> > -            gpiod::gpiod_line_config_get_offsets(
> > -                self.config,
> > -                &mut num as *mut _ as *mut _,
> > -                &mut ptr,
> > -            )
> > -        };
> > -
> > -        if ret == -1 {
> > -            return Err(Error::OperationFailed(
> > -                OperationType::LineConfigGetOffsets,
> > -                errno::errno(),
> > -            ));
> > -        }
> > -
> > -        // SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long
> > -        // as it is not explicitly freed with `free()`.
> > -        let offsets = unsafe { slice::from_raw_parts(ptr as *const Offset, num as usize).to_vec() };
> > -
> > -        // SAFETY: The `ptr` array is guaranteed to be valid here.
> > -        unsafe { libc::free(ptr as *mut c_void) };
> > -
> > -        Ok(offsets)
> > +        Ok(map)
> >      }
> >  }
>
> --
> viresh