diff mbox series

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

Message ID 59f4f17ac2cbe719fa4f571a1c373c36597b12a3.1678314204.git.dxu@dxuuu.xyz
State New
Headers show
Series qga: Support merging output streams in guest-exec | expand

Commit Message

Daniel Xu March 8, 2023, 10:49 p.m. UTC 
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(-)

Comments

Daniel P. Berrangé March 9, 2023, 9:29 a.m. UTC | #1 
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
Daniel Xu March 9, 2023, 4:50 p.m. UTC | #2 
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
diff mbox series

Patch

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
+#
+# Since: 8.0
+##
+ { 'enum': 'GuestExecCaptureOutputMode',
+   'data': [ 'none', 'stdout', 'stderr', 'all' ] }
+
+##
+# @GuestExecCaptureOutput:
+#
+# Controls what guest-exec output gets captures.
+#
+# @flag: captures both stdout and stderr if true. Equivalent
+#        to GuestExecCaptureOutputMode::all. (since 2.5)
+# @mode: capture mode; preferred interface
+#
+# Since: 8.0
+##
+ { 'alternate': 'GuestExecCaptureOutput',
+   'data': { 'flag': 'bool',
+             'mode': 'GuestExecCaptureOutputMode'} }
+
 ##
 # @guest-exec:
 #
@@ -1218,7 +1248,7 @@ 
 ##
 { 'command': 'guest-exec',
   'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
-               '*input-data': 'str', '*capture-output': 'bool' },
+               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput' },
   'returns': 'GuestExec' }