[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/3] qga: Refactor guest-exec capture-output to take enum
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v4 1/3] qga: Refactor guest-exec capture-output to take enum |
Date: |
Thu, 9 Mar 2023 09:29:34 +0000 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
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 :|