Re: [PATCH 1/2] ios: track sub IOS devices

[Jose E. Marchesi]

Hi Mohammad.

Thanks for the patch, and sorry for the delay in reviewing it.
Please see my comments below.

> In each `struct ios' there's a list of all active sub devices.
> This list is used to invalidate sub devices when user closes
> the base IOS.

The general approach sounds good to me.  I tried to keep ios.c unaware
of "sub" IO spaces but unfortunately it is not very convenient :/

>
> 2022-08-10  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
>
>       * bootstrap.conf (libpoke_modules): Add `array-list'.
>       * libpoke/ios.h (ios_sub_claim): New function to track sub
>       IO spaces.
>       (ios_sub_unclaim): Likewise.
>       * libpoke/ios.c (include): Include "gl_array_list.h".
>       (struct ios): Add new field `sub_devs'.
>       (ios_sub_claim): New function definition.
>       (ios_sub_unclaim): Likewise.
>       (ios_open): Intialize `sub_devs' field.
>       (ios_close): Invalidate all active sub IOS devices.
>       * libpoke/ios-dev-sub.c (struct ios_dev_sub): Use `ios' instead of
>       ios id.
>       (ios_dev_sub_open): Update to track base IOS.
>       (ios_dev_sub_close): Likewise.
>       (ios_dev_sub_pread): Use base IOS directly instead of searching
>       using id.
>       (ios_dev_sub_pwrite): Likewise.
>       (ios_dev_sub_invalidate): New global function to report
>       invalidation of a sub IOS device.
> ---
>  ChangeLog             | 22 ++++++++++++++++++++++
>  bootstrap.conf        |  1 +
>  libpoke/ios-dev-sub.c | 29 +++++++++++++++++++----------
>  libpoke/ios.c         | 29 +++++++++++++++++++++++++++++
>  libpoke/ios.h         |  5 +++++
>  5 files changed, 76 insertions(+), 10 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index ef9187f5..834441ee 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,25 @@
> +2022-08-10  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
> +
> +     * bootstrap.conf (libpoke_modules): Add `array-list'.
> +     * libpoke/ios.h (ios_sub_claim): New function to track sub
> +     IO spaces.
> +     (ios_sub_unclaim): Likewise.
> +     * libpoke/ios.c (include): Include "gl_array_list.h".
> +     (struct ios): Add new field `sub_devs'.
> +     (ios_sub_claim): New function definition.
> +     (ios_sub_unclaim): Likewise.
> +     (ios_open): Intialize `sub_devs' field.
> +     (ios_close): Invalidate all active sub IOS devices.
> +     * libpoke/ios-dev-sub.c (struct ios_dev_sub): Use `ios' instead of
> +     ios id.
> +     (ios_dev_sub_open): Update to track base IOS.
> +     (ios_dev_sub_close): Likewise.
> +     (ios_dev_sub_pread): Use base IOS directly instead of searching
> +     using id.
> +     (ios_dev_sub_pwrite): Likewise.
> +     (ios_dev_sub_invalidate): New global function to report
> +     invalidation of a sub IOS device.
> +
>  2022-07-25  Jose E. Marchesi  <jemarch@gnu.org>
>  
>       * configure.ac: Bump version to 2.4.
> diff --git a/bootstrap.conf b/bootstrap.conf
> index 8a16795c..c924a170 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -70,6 +70,7 @@ gnulib_modules="
>  
>  # gnulib modules used in libpoke/.
>  libpoke_modules="
> +  array-list

I'm not a big fan of container-like pre-written data structures unless
there is certain amount of complexity involved, like handling duplicates
and the like.

Since duplicated values are not a concern here, it seems to me that
io->sub_devs is simple enough to be implemented as an array of pointers
reallocated whenever needed.  What do you think about that?

> [...]

> diff --git a/libpoke/ios.h b/libpoke/ios.h
> index a942c43f..478cf030 100644
> --- a/libpoke/ios.h
> +++ b/libpoke/ios.h
> @@ -366,4 +366,9 @@ struct ios_dev_if *ios_foreign_iod (void);
>  struct ios_dev_if;
>  int ios_register_foreign_iod (struct ios_dev_if *iod_if);
>  
> +/* **************** Sub IO space **************** */
> +
> +void ios_sub_claim (ios ios, void *sub_dev);
> +void ios_sub_unclaim (ios ios, void *sub_dev);
> +
>  #endif /* ! IOS_H */

This interface is gonna need documentation in ios.h: what ios_sub_claim
does and what ios_sub_unclaim does.

Thanks!