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

[Daniel Xu]

Hi Daniel,

On Thu, Mar 09, 2023 at 09:29:34AM +0000, Daniel P. Berrangé wrote:
> 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

Sounds good to me.

Thanks,
Daniel