Re: [Qemu-devel] [PATCH v4 09/29] qio: add qio_channel_command_new_spawn_with_pre_exec()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jul 13, 2018 at 03:08:56PM +0200, Marc-André Lureau wrote:
> Add a new function to let caller do some tuning thanks to a callback
> before exec().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/io/channel-command.h | 18 ++++++++++++++++++
>  io/channel-command.c         | 33 ++++++++++++++++++++++++++-------
>  2 files changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/include/io/channel-command.h b/include/io/channel-command.h
> index 336d47fa5c..96c833daab 100644
> --- a/include/io/channel-command.h
> +++ b/include/io/channel-command.h
> @@ -71,6 +71,24 @@ qio_channel_command_new_pid(int writefd,
>                              int readfd,
>                              pid_t pid);
>  
> +/**
> + * qio_channel_command_new_spawn_with_pre_exec:
> + * @argv: the NULL terminated list of command arguments
> + * @flags: the I/O mode, one of O_RDONLY, O_WRONLY, O_RDWR
> + * @errp: pointer to a NULL-initialized error object

Missing the new args

> + *
> + * Create a channel for performing I/O with the
> + * command to be spawned with arguments @argv.
> + *
> + * Returns: the command channel object, or NULL on error
> + */
> +QIOChannelCommand *
> +qio_channel_command_new_spawn_with_pre_exec(const char *const argv[],
> +                                            int flags,
> +                                            void (*pre_exec_cb)(void *),
> +                                            void *data,
> +                                            Error **errp);

I have a slight preference for using a typedef for the callback signature,
and providing API docs explaining the contract.

eg the callback should call _exit() if something happens that it can't
handle

> +
>  /**
>   * qio_channel_command_new_spawn:
>   * @argv: the NULL terminated list of command arguments
> diff --git a/io/channel-command.c b/io/channel-command.c
> index 3e7eb17eff..05903ff194 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -46,9 +46,12 @@ qio_channel_command_new_pid(int writefd,
>  
>  #ifndef WIN32
>  QIOChannelCommand *
> -qio_channel_command_new_spawn(const char *const argv[],
> -                              int flags,
> -                              Error **errp)
> +qio_channel_command_new_spawn_with_pre_exec(const char *const argv[],
> +                                            int flags,
> +                                            void (*pre_exec_cb)(void *),
> +                                            void *data,
> +                                            Error **errp)
> +
>  {
>      pid_t pid = -1;
>      int stdinfd[2] = { -1, -1 };
> @@ -104,6 +107,10 @@ qio_channel_command_new_spawn(const char *const argv[],
>              close(devnull);
>          }
>  
> +        if (pre_exec_cb) {
> +            pre_exec_cb(data);
> +        }
> +
>          execv(argv[0], (char * const *)argv);
>          _exit(1);
>      }
> @@ -139,12 +146,13 @@ qio_channel_command_new_spawn(const char *const argv[],
>      }
>      return NULL;
>  }
> -
>  #else /* WIN32 */
>  QIOChannelCommand *
> -qio_channel_command_new_spawn(const char *const argv[],
> -                              int flags,
> -                              Error **errp)
> +qio_channel_command_new_spawn_with_pre_exec(const char *const argv[],
> +                                            int flags,
> +                                            void (*pre_exec_cb)(void *),
> +                                            void *data,
> +                                            Error **errp)
>  {
>      error_setg_errno(errp, ENOSYS,
>                       "Command spawn not supported on this platform");
> @@ -152,6 +160,17 @@ qio_channel_command_new_spawn(const char *const argv[],
>  }
>  #endif /* WIN32 */
>  
> +
> +QIOChannelCommand *
> +qio_channel_command_new_spawn(const char *const argv[],
> +                              int flags,
> +                              Error **errp)
> +{
> +    return qio_channel_command_new_spawn_with_pre_exec(argv, flags,
> +                                                       NULL, NULL, errp);
> +}
> +
> +
>  #ifndef WIN32
>  static int qio_channel_command_abort(QIOChannelCommand *ioc,
>                                       Error **errp)
> -- 
> 2.18.0.129.ge3331758f1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|