Re: [PATCH v4 1/3] qga: Refactor guest-exec capture-output to take enum

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

On Wed, Mar 08, 2023 at 03:49:39PM -0700, Daniel Xu wrote:
> Previously capture-output was an optional boolean flag that either
> captured all output or captured none. While this is OK in most cases, it
> lacks flexibility for more advanced capture cases, such as wanting to
> only capture stdout.
> 
> This commits refactors guest-exec qapi to take an enum for capture mode
> instead while preserving backwards compatibility.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  qga/commands.c       | 37 ++++++++++++++++++++++++++++++++++---
>  qga/qapi-schema.json | 32 +++++++++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index 172826f8f8..5504fc5b8c 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -379,11 +379,23 @@ close:
>      return false;
>  }
>  
> +static GuestExecCaptureOutputMode ga_parse_capture_output(
> +        GuestExecCaptureOutput *capture_output)
> +{
> +    if (!capture_output)
> +        return GUEST_EXEC_CAPTURE_OUTPUT_MODE_NONE;
> +    else if (capture_output->type == QTYPE_QBOOL)
> +        return capture_output->u.flag ? GUEST_EXEC_CAPTURE_OUTPUT_MODE_ALL
> +                                      : GUEST_EXEC_CAPTURE_OUTPUT_MODE_NONE;
> +    else
> +        return capture_output->u.mode;
> +}
> +
>  GuestExec *qmp_guest_exec(const char *path,
>                         bool has_arg, strList *arg,
>                         bool has_env, strList *env,
>                         const char *input_data,
> -                       bool has_capture_output, bool capture_output,
> +                       GuestExecCaptureOutput *capture_output,
>                         Error **errp)
>  {
>      GPid pid;
> @@ -396,7 +408,8 @@ GuestExec *qmp_guest_exec(const char *path,
>      gint in_fd, out_fd, err_fd;
>      GIOChannel *in_ch, *out_ch, *err_ch;
>      GSpawnFlags flags;
> -    bool has_output = (has_capture_output && capture_output);
> +    bool has_output = false;
> +    GuestExecCaptureOutputMode output_mode;
>      g_autofree uint8_t *input = NULL;
>      size_t ninput = 0;
>  
> @@ -415,8 +428,26 @@ GuestExec *qmp_guest_exec(const char *path,
>  
>      flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD |
>          G_SPAWN_SEARCH_PATH_FROM_ENVP;
> -    if (!has_output) {
> +
> +    output_mode = ga_parse_capture_output(capture_output);
> +    switch (output_mode) {
> +    case GUEST_EXEC_CAPTURE_OUTPUT_MODE_NONE:
>          flags |= G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
> +        break;
> +    case GUEST_EXEC_CAPTURE_OUTPUT_MODE_STDOUT:
> +        has_output = true;
> +        flags |= G_SPAWN_STDERR_TO_DEV_NULL;
> +        break;
> +    case GUEST_EXEC_CAPTURE_OUTPUT_MODE_STDERR:
> +        has_output = true;
> +        flags |= G_SPAWN_STDOUT_TO_DEV_NULL;
> +        break;
> +    case GUEST_EXEC_CAPTURE_OUTPUT_MODE_ALL:
> +        has_output = true;
> +        break;
> +    case GUEST_EXEC_CAPTURE_OUTPUT_MODE__MAX:
> +        /* Silence warning; impossible branch */
> +        break;
>      }
>  
>      ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 796434ed34..4ef585da5d 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1200,6 +1200,36 @@
>  { 'struct': 'GuestExec',
>    'data': { 'pid': 'int'} }
>  
> +##
> +# @GuestExecCaptureOutputMode:
> +#
> +# An enumeration of guest-exec capture modes.
> +#
> +# @none: do not capture any output
> +# @stdout: only capture stdout
> +# @stderr: only capture stderr
> +# @all: capture both stdout and stderr

Functionally fine. A minor naming thought.... How about we call
this '@separated' and tweak the docs

  @separated: capture both stdout and stderr separately

Then in your in next patch you introduce

  @merged: capture both stdout and stderr merged

With 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 :|