Re: [PATCH v3] pkl: Add data payload param in IO device API

[Jose E. Marchesi]

Hi Mohammad.

> 2021-12-23  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
>
>       * libpoke/ios-dev-file.c (ios_dev_file_open): Add new param `data`.
>       * libpoke/ios-dev-mem.c (ios_dev_mem_open): Likewise.
>       * libpoke/ios-dev-nbd.c (ios_dev_nbd_open): Likewise.
>       * libpoke/ios-dev-proc.c (ios_dev_proc_open): Likewise.
>       * libpoke/ios-dev-stream.c (ios_dev_stream_open): Likewise.
>       * libpoke/ios-dev-zero.c (ios_dev_zero_open): Likewise.
>       * libpoke/ios-dev-sub.c (ios_dev_sub_open): Likewise.
>       * libpoke/ios-dev.h (struct ios_dev_if): Change `open` to accept new
>       `data` param. Add new field `data`.
>       * libpoke/libpoke.h (struct pk_iod_if): Likewise.
>       * libpoke/ios.c (ios_open): Call `open` with IOD-specific data.
>       * testsuite/poke.libpoke/foreign-iod.c (iod_op): Add new field
>       `data`.
>       (iod_open): Add new param `data`. Report success through `error`.
>       (iod_if): Add user data.
>       (USER_DATA): New constant.
>       (main): Add test for user data.
> ---
>
> Hi, Jose.
>
> - I used __attribute__((unused)) to avoid warnings.
> - Improved the doc string.
> - In `libpoke/ios-dev.h`, we need an empty line to be consistent with
>   the rest of fields.
>   I removed the empty line in `libpoke/libpoke.h`.
> - I changed the `user_data` to `data`. IMO it's a better name. As you
>   said it's a IOD-specific data, not user data.

Yes I agree, thats a better name.

Thanks for doing the changes.
This is OK for master!

>
>
> Regards,
> Mohammad-Reza
>
>
>  ChangeLog                            | 20 ++++++++++++++++++++
>  libpoke/ios-dev-file.c               |  3 ++-
>  libpoke/ios-dev-mem.c                |  3 ++-
>  libpoke/ios-dev-nbd.c                |  3 ++-
>  libpoke/ios-dev-proc.c               |  5 +++--
>  libpoke/ios-dev-stream.c             |  3 ++-
>  libpoke/ios-dev-sub.c                |  2 +-
>  libpoke/ios-dev-zero.c               |  4 +++-
>  libpoke/ios-dev.h                    | 13 ++++++++++---
>  libpoke/ios.c                        |  2 +-
>  libpoke/libpoke.c                    |  1 +
>  libpoke/libpoke.h                    |  3 ++-
>  testsuite/poke.libpoke/foreign-iod.c | 16 ++++++++++++++--
>  13 files changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 1aa44925..74c40cdf 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,23 @@
> +2021-12-23  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
> +
> +     * libpoke/ios-dev-file.c (ios_dev_file_open): Add new param `data`.
> +     * libpoke/ios-dev-mem.c (ios_dev_mem_open): Likewise.
> +     * libpoke/ios-dev-nbd.c (ios_dev_nbd_open): Likewise.
> +     * libpoke/ios-dev-proc.c (ios_dev_proc_open): Likewise.
> +     * libpoke/ios-dev-stream.c (ios_dev_stream_open): Likewise.
> +     * libpoke/ios-dev-zero.c (ios_dev_zero_open): Likewise.
> +     * libpoke/ios-dev-sub.c (ios_dev_sub_open): Likewise.
> +     * libpoke/ios-dev.h (struct ios_dev_if): Change `open` to accept new
> +     `data` param. Add new field `data`.
> +     * libpoke/libpoke.h (struct pk_iod_if): Likewise.
> +     * libpoke/ios.c (ios_open): Call `open` with IOD-specific data.
> +     * testsuite/poke.libpoke/foreign-iod.c (iod_op): Add new field
> +     `data`.
> +     (iod_open): Add new param `data`. Report success through `error`.
> +     (iod_if): Add user data.
> +     (USER_DATA): New constant.
> +     (main): Add test for user data.
> +
>  2021-12-23  Jose E. Marchesi  <jemarch@gnu.org>
>  
>       * etc/poke.rec (.set with no arguments should give a summary of
> diff --git a/libpoke/ios-dev-file.c b/libpoke/ios-dev-file.c
> index d470c62e..31ea0de9 100644
> --- a/libpoke/ios-dev-file.c
> +++ b/libpoke/ios-dev-file.c
> @@ -96,7 +96,8 @@ ios_dev_file_convert_flags (int mode_flags, char 
> **mode_for_fdopen)
>  }
>  
>  static void *
> -ios_dev_file_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_file_open (const char *handler, uint64_t flags, int *error,
> +                   void *data __attribute__ ((unused)))
>  {
>    struct ios_dev_file *fio = NULL;
>    FILE *f = NULL;
> diff --git a/libpoke/ios-dev-mem.c b/libpoke/ios-dev-mem.c
> index c1757500..f2b8b042 100644
> --- a/libpoke/ios-dev-mem.c
> +++ b/libpoke/ios-dev-mem.c
> @@ -56,7 +56,8 @@ ios_dev_mem_handler_normalize (const char *handler, 
> uint64_t flags, int *error)
>  }
>  
>  static void *
> -ios_dev_mem_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_mem_open (const char *handler, uint64_t flags, int *error,
> +                  void *data __attribute__ ((unused)))
>  {
>    int internal_error = IOD_ERROR;
>    struct ios_dev_mem *mio = malloc (sizeof (struct ios_dev_mem));
> diff --git a/libpoke/ios-dev-nbd.c b/libpoke/ios-dev-nbd.c
> index 2c3fa6ab..00f74a1b 100644
> --- a/libpoke/ios-dev-nbd.c
> +++ b/libpoke/ios-dev-nbd.c
> @@ -66,7 +66,8 @@ ios_dev_nbd_handler_normalize (const char *handler, 
> uint64_t flags, int* error)
>  }
>  
>  static void *
> -ios_dev_nbd_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_nbd_open (const char *handler, uint64_t flags, int *error,
> +                  void *data __attribute__ ((unused)))
>  {
>    struct ios_dev_nbd *nio = NULL;
>    struct nbd_handle *nbd = NULL;
> diff --git a/libpoke/ios-dev-proc.c b/libpoke/ios-dev-proc.c
> index a6ad9247..67b5c616 100644
> --- a/libpoke/ios-dev-proc.c
> +++ b/libpoke/ios-dev-proc.c
> @@ -71,7 +71,8 @@ ios_dev_proc_handler_normalize (const char *handler, 
> uint64_t flags, int *error)
>  }
>  
>  static void *
> -ios_dev_proc_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_proc_open (const char *handler, uint64_t flags, int *error,
> +                   void *data __attribute__ ((unused)))
>  {
>    struct ios_dev_proc *proc = malloc (sizeof (struct ios_dev_proc));
>  
> @@ -112,7 +113,7 @@ ios_dev_proc_open (const char *handler, uint64_t flags, 
> int *error)
>  
>    proc->memfile = ios_dev_file.open (proc->memfile_path,
>                                       IOS_F_READ | IOS_F_WRITE,
> -                                     error);
> +                                     error, NULL);
>    if (proc->memfile == NULL)
>      {
>        free (proc);
> diff --git a/libpoke/ios-dev-stream.c b/libpoke/ios-dev-stream.c
> index 5a566657..64b15339 100644
> --- a/libpoke/ios-dev-stream.c
> +++ b/libpoke/ios-dev-stream.c
> @@ -70,7 +70,8 @@ ios_dev_stream_handler_normalize (const char *handler, 
> uint64_t flags, int *erro
>  }
>  
>  static void *
> -ios_dev_stream_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_stream_open (const char *handler, uint64_t flags, int *error,
> +                     void *data __attribute__ ((unused)))
>  {
>    struct ios_dev_stream *sio;
>    int internal_error = IOD_ERROR;
> diff --git a/libpoke/ios-dev-sub.c b/libpoke/ios-dev-sub.c
> index fe061511..f6646dab 100644
> --- a/libpoke/ios-dev-sub.c
> +++ b/libpoke/ios-dev-sub.c
> @@ -68,7 +68,7 @@ ios_dev_sub_handler_normalize (const char *handler, 
> uint64_t flags, int* error)
>  }
>  
>  static void *
> -ios_dev_sub_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_sub_open (const char *handler, uint64_t flags, int *error, void 
> *data)
>  {
>    struct ios_dev_sub *sub = malloc (sizeof (struct ios_dev_sub));
>    const char *p;
> diff --git a/libpoke/ios-dev-zero.c b/libpoke/ios-dev-zero.c
> index 2772c81d..3c8849b6 100644
> --- a/libpoke/ios-dev-zero.c
> +++ b/libpoke/ios-dev-zero.c
> @@ -53,7 +53,9 @@ ios_dev_zero_handler_normalize (const char *handler, 
> uint64_t flags, int *error)
>  }
>  
>  static void *
> -ios_dev_zero_open (const char *handler, uint64_t flags, int *error)
> +ios_dev_zero_open (const char *handler __attribute__ ((unused)),
> +                   uint64_t flags __attribute__ ((unused)), int *error,
> +                   void *data __attribute__ ((unused)))
>  {
>    /* This IOD doesn't need to keep any state.  */
>    if (error)
> diff --git a/libpoke/ios-dev.h b/libpoke/ios-dev.h
> index 2ffd5fea..f563a9b0 100644
> --- a/libpoke/ios-dev.h
> +++ b/libpoke/ios-dev.h
> @@ -65,9 +65,10 @@ struct ios_dev_if
>    /* Open a device using the provided HANDLER.  Return the opened
>       device, or NULL in case of errors.  Set the ERROR to error code or to
>       IOD_OK.  Note that this function assumes that HANDLER is recognized as a
> -     handler by the backend.  */
> +     handler by the backend.  DATA is the `data' pointer below, intended to 
> be
> +     used as an IOD-specific payload specified by the author of IOD.  */
>  
> -  void * (*open) (const char *handler, uint64_t flags, int *error);
> +  void * (*open) (const char *handler, uint64_t flags, int *error, void 
> *data);
>  
>    /* Close the given device.  Return the error code if there was an error
>       during the operation, IOD_OK otherwise.  */
> @@ -94,9 +95,15 @@ struct ios_dev_if
>  
>    /* If called on a in-stream, free the buffer before OFFSET.  If called on
>       an out-stream, flush the data till OFFSET and free the buffer before
> -     OFFSET.  Otherwise, do not do anything.  Return IOS_OK ın success and
> +     OFFSET.  Otherwise, do not do anything.  Return IOS_OK in success and
>       an error code on failure.  */
> +
>    int (*flush) (void *dev, ios_dev_off offset);
> +
> +  /* IOD-specific data payload that will be passed to OPEN function.  If not
> +     used, it should be NULL.  */
> +
> +  void *data;
>  };
>  
>  #define IOS_FILE_HANDLER_NORMALIZE(handler, new_handler)                \
> diff --git a/libpoke/ios.c b/libpoke/ios.c
> index b12f5e57..6bc5bd12 100644
> --- a/libpoke/ios.c
> +++ b/libpoke/ios.c
> @@ -186,7 +186,7 @@ ios_open (const char *handler, uint64_t flags, int 
> set_cur)
>        }
>  
>    /* Open the device using the interface found above.  */
> -  io->dev = io->dev_if->open (handler, flags, &iod_error);
> +  io->dev = io->dev_if->open (handler, flags, &iod_error, io->dev_if->data);
>    if (iod_error || io->dev == NULL)
>      goto error;
>  
> diff --git a/libpoke/libpoke.c b/libpoke/libpoke.c
> index 072205cf..6ae3ee41 100644
> --- a/libpoke/libpoke.c
> +++ b/libpoke/libpoke.c
> @@ -974,6 +974,7 @@ pk_register_iod (pk_compiler pkc, struct pk_iod_if 
> *iod_if)
>    CF (get_flags);
>    CF (size);
>    CF (flush);
> +  CF (data);
>  #undef CF
>  
>    (void) ios_register_foreign_iod (&foreign_iod_if);
> diff --git a/libpoke/libpoke.h b/libpoke/libpoke.h
> index 6332eef2..394c2794 100644
> --- a/libpoke/libpoke.h
> +++ b/libpoke/libpoke.h
> @@ -1074,13 +1074,14 @@ struct pk_iod_if
>  {
>    const char *(*get_if_name) ();
>    char *(*handler_normalize) (const char *handler, uint64_t flags, int 
> *error);
> -  void * (*open) (const char *handler, uint64_t flags, int *error);
> +  void * (*open) (const char *handler, uint64_t flags, int *error, void 
> *data);
>    int (*close) (void *dev);
>    int (*pread) (void *dev, void *buf, size_t count, pk_iod_off offset);
>    int (*pwrite) (void *dev, const void *buf, size_t count, pk_iod_off 
> offset);
>    uint64_t (*get_flags) (void *dev);
>    pk_iod_off (*size) (void *dev);
>    int (*flush) (void *dev, pk_iod_off offset);
> +  void *data;
>  };
>  
>  /* Register a foreign IO device in the Poke compiler.
> diff --git a/testsuite/poke.libpoke/foreign-iod.c 
> b/testsuite/poke.libpoke/foreign-iod.c
> index 0c97ae44..57f01d0e 100644
> --- a/testsuite/poke.libpoke/foreign-iod.c
> +++ b/testsuite/poke.libpoke/foreign-iod.c
> @@ -51,6 +51,7 @@ struct
>    int type; /* IOD_OP_READ or IOD_OP_WRITE */
>    pk_iod_off offset;
>    size_t count;
> +  void *data;
>  } iod_op;
>  
>  int iod_opened_p = 0;
> @@ -81,9 +82,12 @@ iod_handler_normalize (const char *handler, uint64_t 
> flags, int *error)
>  }
>  
>  void *
> -iod_open (const char *handler, uint64_t flags, int *error)
> +iod_open (const char *handler, uint64_t flags, int *error, void *data)
>  {
>    iod_opened_p = 1;
> +  iod_op.data = data;
> +  if (error)
> +    *error = PK_IOD_OK;
>    return &iod_opened_p;
>  }
>  
> @@ -136,6 +140,8 @@ iod_close (void *dev)
>    return PK_OK;
>  }
>  
> +static uint32_t USER_DATA = 0x706f6b65u; /* "poke" */
> +
>  struct pk_iod_if iod_if =
>    {
>      iod_get_if_name,
> @@ -146,7 +152,8 @@ struct pk_iod_if iod_if =
>      iod_pwrite,
>      iod_get_flags,
>      iod_size,
> -    iod_flush
> +    iod_flush,
> +    (void*)&USER_DATA,
>    };
>  
>  int
> @@ -183,6 +190,11 @@ main (int argc, char *argv[])
>          fail ("open not reflected in foreign IOD");
>          return 1;
>        }
> +    if (*(uint32_t*)iod_op.data != USER_DATA)
> +      {
> +        fail ("user data not reflected in foreign IOD");
> +        return 1;
> +      }
>  
>      pass ("open foreign IOD");
>    }