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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

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 ?

> +
> +        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