Re: [PATCH 1/3] Take error as an argument rather than having it as return value.

[Jose E. Marchesi]

Hi Egeyar.

> 2021-02-24  Egeyar Bagcioglu  <egeyar@gmail.com>
>
>         * libpoke/ios-dev-file.c (ios_dev_file_handler_normalize): Take error
>         as an argument rather than having it as return value.
>         (ios_dev_file_open): Likewise.
>         * libpoke/ios-dev-mem.c (ios_dev_mem_handler_normalize): Likewise.
>         (ios_dev_mem_open): Likewise.
>         * libpoke/ios-dev-nbd.c (ios_dev_nbd_handler_normalize): Likewise.
>         (ios_dev_nbd_open): Likewise.
>         * libpoke/ios-dev-stream.c (ios_dev_stream_handler_normalize):
>         Likewise.
>         (ios_dev_stream_open): Likewise.
>         * libpoke/ios-dev.h (handler_normalize): Likewise.
>         (open): Likewise.
>         (IOS_FILE_HANDLER_NORMALIZE): Rename newhandler as new_handler.
>         * ios.c (ios_open): Adjust calls to handler_normalize and open.

Very nice.
I like the uniformity of the new API very much :)

OK for master.
Thanks!

> ---
>  ChangeLog                | 17 ++++++++++++
>  libpoke/ios-dev-file.c   | 56 ++++++++++++++++++++++++++--------------
>  libpoke/ios-dev-mem.c    | 40 ++++++++++++++++++----------
>  libpoke/ios-dev-nbd.c    | 39 ++++++++++++++++------------
>  libpoke/ios-dev-stream.c | 46 +++++++++++++++++++--------------
>  libpoke/ios-dev.h        | 27 ++++++++++---------
>  libpoke/ios.c            |  8 +++---
>  7 files changed, 144 insertions(+), 89 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 93edebbf..ea3091c4 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,20 @@
> +2021-02-24  Egeyar Bagcioglu  <egeyar@gmail.com>
> +
> +     * libpoke/ios-dev-file.c (ios_dev_file_handler_normalize): Take error
> +     as an argument rather than having it as return value.
> +     (ios_dev_file_open): Likewise.
> +     * libpoke/ios-dev-mem.c (ios_dev_mem_handler_normalize): Likewise.
> +     (ios_dev_mem_open): Likewise.
> +     * libpoke/ios-dev-nbd.c (ios_dev_nbd_handler_normalize): Likewise.
> +     (ios_dev_nbd_open): Likewise.
> +     * libpoke/ios-dev-stream.c (ios_dev_stream_handler_normalize):
> +     Likewise.
> +     (ios_dev_stream_open): Likewise.
> +     * libpoke/ios-dev.h (handler_normalize): Likewise.
> +     (open): Likewise.
> +     (IOS_FILE_HANDLER_NORMALIZE): Rename newhandler as new_handler.
> +     * ios.c (ios_open): Adjust calls to handler_normalize and open.
> +
>  2021-02-24  Egeyar  Bagcioglu  <egeyar@gmail.com>
>  
>       * libpoke/ios-dev-stream.c (ios_dev_stream_pread): Change internal
> diff --git a/libpoke/ios-dev-file.c b/libpoke/ios-dev-file.c
> index 35284541..5584ed37 100644
> --- a/libpoke/ios-dev-file.c
> +++ b/libpoke/ios-dev-file.c
> @@ -46,23 +46,29 @@ ios_dev_file_get_if_name () {
>    return "FILE";
>  }
>  
> -static int
> -ios_dev_file_handler_normalize (const char *handler, uint64_t flags, char 
> **newhandler)
> +static char *
> +ios_dev_file_handler_normalize (const char *handler, uint64_t flags, int* 
> error)
>  {
> -  IOS_FILE_HANDLER_NORMALIZE (handler, *newhandler);
> +  char *new_handler = NULL;
> +  IOS_FILE_HANDLER_NORMALIZE (handler, new_handler);
>  
> -  /* Every handler is accepted as a file handler.  */
> -  if (*newhandler == NULL)
> -    return IOD_ENOMEM;
> -  else
> -    return IOD_OK;
> +  if (error)
> +    {
> +      if (new_handler == NULL)
> +        *error = IOD_ENOMEM;
> +      else
> +        *error = IOD_OK;
> +    }
> +
> +  return new_handler;
>  }
>  
> -static int
> -ios_dev_file_open (const char *handler, uint64_t flags, void **dev)
> +static void *
> +ios_dev_file_open (const char *handler, uint64_t flags, int *error)
>  {
>    struct ios_dev_file *fio = NULL;
>    FILE *f;
> +  int internal_error = IOD_ERROR;
>    const char *mode;
>    uint8_t flags_mode = flags & IOS_FLAGS_MODE;
>  
> @@ -78,7 +84,10 @@ ios_dev_file_open (const char *handler, uint64_t flags, 
> void **dev)
>        else if (flags_mode == (IOS_F_WRITE | IOS_F_CREATE | IOS_F_TRUNCATE))
>          mode = "w+b";
>        else
> -          return IOD_EFLAGS;
> +        {
> +          internal_error = IOD_EFLAGS;
> +          goto err;
> +        }
>  
>        f = fopen (handler, mode);
>      }
> @@ -109,8 +118,9 @@ ios_dev_file_open (const char *handler, uint64_t flags, 
> void **dev)
>    fio->file = f;
>    fio->flags = flags;
>  
> -  *dev = fio;
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return fio;
>  
>  err:
>    if (fio)
> @@ -120,13 +130,19 @@ err:
>    if (f)
>      fclose (f);
>  
> -  if (errno == ENOMEM)
> -    return IOD_ENOMEM;
> -  else if (errno == EINVAL)
> -    return IOD_EINVAL;
> -  /* Should never happen.  */
> -  else
> -    return IOD_ERROR;
> +  if (error)
> +    {
> +      if (internal_error != IOD_ERROR)
> +        *error = internal_error;
> +      else if (errno == ENOMEM)
> +        *error = IOD_ENOMEM;
> +      else if (errno == EINVAL)
> +        *error = IOD_EINVAL;
> +      /* Should never happen.  */
> +      else
> +        *error = IOD_ERROR;
> +    }
> +  return NULL;
>  }
>  
>  static int
> diff --git a/libpoke/ios-dev-mem.c b/libpoke/ios-dev-mem.c
> index 4263ca9b..9d853bda 100644
> --- a/libpoke/ios-dev-mem.c
> +++ b/libpoke/ios-dev-mem.c
> @@ -40,38 +40,52 @@ ios_dev_mem_get_if_name () {
>    return "MEMORY";
>  }
>  
> -static int
> -ios_dev_mem_handler_normalize (const char *handler, uint64_t flags, char 
> **newhandler)
> +static char *
> +ios_dev_mem_handler_normalize (const char *handler, uint64_t flags, int 
> *error)
>  {
> +  char *new_handler = NULL;
>    if (handler[0] == '*' && handler[strlen (handler) - 1] == '*')
>      {
> -      *newhandler = strdup (handler);
> -      if (*newhandler == NULL)
> -        return IOD_ENOMEM;
> +      new_handler = strdup (handler);
> +      if (new_handler == NULL && error)
> +        *error = IOD_ENOMEM;
>      }
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return new_handler;
>  }
>  
> -static int
> -ios_dev_mem_open (const char *handler, uint64_t flags, void **dev)
> +static void *
> +ios_dev_mem_open (const char *handler, uint64_t flags, int *error)
>  {
> +  int internal_error = IOD_ERROR;
>    struct ios_dev_mem *mio = malloc (sizeof (struct ios_dev_mem));
>  
>    if (!mio)
> -    return IOD_ENOMEM;
> +    {
> +      internal_error = IOD_ENOMEM;
> +      goto err;
> +    }
>  
>    mio->pointer = calloc (MEM_STEP, 1);
>    if (!mio->pointer)
>      {
> -      free (mio);
> -      return IOD_ENOMEM;
> +      internal_error = IOD_ENOMEM;
> +      goto err;
>      }
>  
>    mio->size = MEM_STEP;
>    mio->flags = flags;
>  
> -  *dev = mio;
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return mio;
> +
> +err:
> +  free (mio);
> +  if (error)
> +    *error = internal_error;
> +  return NULL;
>  }
>  
>  static int
> diff --git a/libpoke/ios-dev-nbd.c b/libpoke/ios-dev-nbd.c
> index 7d89b763..2ff65c39 100644
> --- a/libpoke/ios-dev-nbd.c
> +++ b/libpoke/ios-dev-nbd.c
> @@ -49,32 +49,35 @@ ios_dev_nbd_get_if_name () {
>    return "NBD";
>  }
>  
> -static int
> -ios_dev_nbd_handler_normalize (const char *handler, uint64_t flags, char 
> **newhandler)
> +static char *
> +ios_dev_nbd_handler_normalize (const char *handler, uint64_t flags, int* 
> error)
>  {
> +  char * new_handler = NULL;
>    if (startswith (handler, "nbd://")
>        || startswith (handler, "nbd+unix://"))
>      {
> -      *newhandler = strdup (handler);
> -      if (*newhandler == NULL)
> -        return IOD_ENOMEM;
> +      new_handler = strdup (handler);
> +      if (new_handler == NULL && error)
> +        *error = IOD_ENOMEM;
>      }
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return new_handler;
>  }
>  
> -static int
> -ios_dev_nbd_open (const char *handler, uint64_t flags, void **dev)
> +static void *
> +ios_dev_nbd_open (const char *handler, uint64_t flags, int *error)
>  {
>    struct ios_dev_nbd *nio = NULL;
>    struct nbd_handle *nbd = NULL;
>    uint8_t flags_mode = flags & IOS_FLAGS_MODE;
> -  int err = IOD_ERROR;
> +  int internal_error = IOD_ERROR;
>    int64_t size;
>  
>    /* We don't permit truncation.  */
>    if (flags_mode & IOS_F_TRUNCATE)
>      {
> -      err = IOD_EFLAGS;
> +      internal_error = IOD_EFLAGS;
>        goto err;
>      }
>  
> @@ -88,7 +91,7 @@ ios_dev_nbd_open (const char *handler, uint64_t flags, void 
> **dev)
>  
>    if (flags_mode & IOS_F_WRITE && nbd_is_read_only (nbd))
>      {
> -      err = IOD_EINVAL;
> +      internal_error = IOD_EINVAL;
>        goto err;
>      }
>    else if (flags_mode == 0)
> @@ -105,14 +108,14 @@ ios_dev_nbd_open (const char *handler, uint64_t flags, 
> void **dev)
>    nio = malloc (sizeof *nio);
>    if (!nio)
>      {
> -      err = IOD_ENOMEM;
> +      internal_error = IOD_ENOMEM;
>        goto err;
>      }
>  
>    nio->uri = strdup (handler);
>    if (!nio->uri)
>      {
> -      err = IOD_ENOMEM;
> +      internal_error = IOD_ENOMEM;
>        goto err;
>      }
>  
> @@ -120,8 +123,9 @@ ios_dev_nbd_open (const char *handler, uint64_t flags, 
> void **dev)
>    nio->size = size;
>    nio->flags = flags;
>  
> -  *dev = nio;
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return nio;
>  
>   err:
>    /* Worth logging nbd_get_error ()?  */
> @@ -130,8 +134,9 @@ ios_dev_nbd_open (const char *handler, uint64_t flags, 
> void **dev)
>    free (nio);
>  
>    nbd_close (nbd);
> -
> -  return err;
> +  if (error)
> +    *error = internal_error;
> +  return NULL;
>  }
>  
>  static int
> diff --git a/libpoke/ios-dev-stream.c b/libpoke/ios-dev-stream.c
> index 81f0489a..5184c388 100644
> --- a/libpoke/ios-dev-stream.c
> +++ b/libpoke/ios-dev-stream.c
> @@ -51,39 +51,42 @@ ios_dev_stream_get_dev_if_name () {
>    return "STREAM";
>  }
>  
> -static int
> -ios_dev_stream_handler_normalize (const char *handler, uint64_t flags, char 
> **newhandler)
> +static char *
> +ios_dev_stream_handler_normalize (const char *handler, uint64_t flags, int 
> *error)
>  {
> +  char *new_handler = NULL;
>    /* TODO handle the case where strdup fails. */
>    if (STREQ (handler, IOS_STDIN_HANDLER)
>        || STREQ (handler, IOS_STDOUT_HANDLER)
>        || STREQ (handler, IOS_STDERR_HANDLER))
>      {
> -      *newhandler = strdup (handler);
> -      if (*newhandler == NULL)
> -        return IOD_ENOMEM;
> +      new_handler = strdup (handler);
> +      if (new_handler == NULL && error)
> +        *error = IOD_ENOMEM;
>      }
> -  return IOD_OK;
> +  if (error)
> +    *error = IOD_OK;
> +  return new_handler;
>  }
>  
> -static int
> -ios_dev_stream_open (const char *handler, uint64_t flags, void **dev)
> +static void *
> +ios_dev_stream_open (const char *handler, uint64_t flags, int *error)
>  {
>    struct ios_dev_stream *sio;
> -  int error = IOD_ERROR;
> +  int internal_error = IOD_ERROR;
>  
>    sio = malloc (sizeof (struct ios_dev_stream));
>    if (!sio)
>      {
> -      error = IOD_ENOMEM;
> -      goto error;
> +      internal_error = IOD_ENOMEM;
> +      goto err;
>      }
>  
>    sio->handler = strdup (handler);
>    if (!sio->handler)
>      {
> -      error = IOD_ENOMEM;
> -      goto error;
> +      internal_error = IOD_ENOMEM;
> +      goto err;
>      }
>  
>    if (STREQ (handler, IOS_STDIN_HANDLER))
> @@ -93,8 +96,8 @@ ios_dev_stream_open (const char *handler, uint64_t flags, 
> void **dev)
>        sio->buffer = ios_buffer_init ();
>        if (!sio->buffer)
>          {
> -          error = IOD_ENOMEM;
> -          goto error;
> +          internal_error = IOD_ENOMEM;
> +          goto err;
>          }
>      }
>    else if (STREQ (handler, IOS_STDOUT_HANDLER))
> @@ -110,17 +113,20 @@ ios_dev_stream_open (const char *handler, uint64_t 
> flags, void **dev)
>        sio->write_offset = 0;
>      }
>    else
> -    goto error;
> +    goto err;
>  
> -  *dev = sio;
> -  return IOD_OK;
> +  if (error)
> +   *error = IOD_OK;
> +  return sio;
>  
> -error:
> +err:
>    if (sio)
>      free (sio->handler);
>    free (sio);
>  
> -  return error;
> +  if (error)
> +    *error = internal_error;
> +  return NULL;
>  }
>  
>  static int
> diff --git a/libpoke/ios-dev.h b/libpoke/ios-dev.h
> index 127ae846..4fbbcc1a 100644
> --- a/libpoke/ios-dev.h
> +++ b/libpoke/ios-dev.h
> @@ -53,21 +53,20 @@ struct ios_dev_if
>    char *(*get_if_name) ();
>  
>    /* Determine whether the provided HANDLER is recognized as a valid
> -     device spec by this backend, and if so, copy its normalized
> -     form into newhandler (caller will free).  In case of error,
> -     return the error code.  If not, return IOD_OK.  */
> +     device spec by this backend, and if so, return its normalized
> +     form (caller will free).  In case of error, return NULL.  This function
> +     sets the ERROR to error code or to IOD_OK.  */
>  
> -  int (*handler_normalize) (const char *handler, uint64_t flags, char 
> **newhandler);
> +  char * (*handler_normalize) (const char *handler, uint64_t flags, int* 
> error);
>  
> -  /* Open a device using the provided HANDLER.  Fill DEV with the opened
> -     device, or return the error code.  Note that this function assumes that
> -     HANDLER is recognized as a handler by the backend, i.e. HANDLER_P
> -     returns 1 if HANDLER is passed to it.  */
> +  /* 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.  */
>  
> -  int (*open) (const char *handler, uint64_t flags, void **dev);
> +  void * (*open) (const char *handler, uint64_t flags, int *error);
>  
>    /* Close the given device.  Return 0 if there was an error during
> -
>       the operation, 1 otherwise.  */
>  
>    int (*close) (void *dev);
> @@ -97,7 +96,7 @@ struct ios_dev_if
>    int (*flush) (void *dev, ios_dev_off offset);
>  };
>  
> -#define IOS_FILE_HANDLER_NORMALIZE(handler, newhandler)                 \
> +#define IOS_FILE_HANDLER_NORMALIZE(handler, new_handler)                \
>    do                                                                    \
>      {                                                                   \
>        /* File devices are special, in the sense that they accept any */ \
> @@ -111,8 +110,8 @@ struct ios_dev_if
>                                                                          \
>        if (handler[0] == '/'                                             \
>            || strspn (handler, safe) == strlen (handler))                \
> -        (newhandler) = strdup ((handler));                              \
> -      else if (asprintf (&(newhandler), "./%s", (handler)) == -1)       \
> -        (newhandler) = NULL;                                            \
> +        (new_handler) = strdup ((handler));                             \
> +      else if (asprintf (&(new_handler), "./%s", (handler)) == -1)      \
> +        (new_handler) = NULL;                                           \
>      }                                                                   \
>    while (0)
> diff --git a/libpoke/ios.c b/libpoke/ios.c
> index 7da34bcf..8db45f5b 100644
> --- a/libpoke/ios.c
> +++ b/libpoke/ios.c
> @@ -130,8 +130,6 @@ ios_open (const char *handler, uint64_t flags, int 
> set_cur)
>    if (!io)
>      return IOS_ENOMEM;
>  
> -  io->handler = NULL;
> -  io->dev = NULL;
>    io->next = NULL;
>    io->bias = 0;
>  
> @@ -139,8 +137,8 @@ ios_open (const char *handler, uint64_t flags, int 
> set_cur)
>       handler.  */
>    for (dev_if = ios_dev_ifs; *dev_if; ++dev_if)
>      {
> -      iod_error = (*dev_if)->handler_normalize (handler, flags, 
> &io->handler);
> -      if (iod_error)
> +      io->handler = (*dev_if)->handler_normalize (handler, flags, 
> &iod_error);
> +      if (iod_error != IOD_OK)
>          goto error;
>        if (io->handler)
>          break;
> @@ -160,7 +158,7 @@ ios_open (const char *handler, uint64_t flags, int 
> set_cur)
>        }
>  
>    /* Open the device using the interface found above.  */
> -  iod_error = io->dev_if->open (handler, flags, &io->dev);
> +  io->dev = io->dev_if->open (handler, flags, &iod_error);
>    if (iod_error || io->dev == NULL)
>      goto error;