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é <berra...@redhat.com> > > Signed-off-by: Daniel Xu <d...@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