Re: [PATCH 2/3] qga: Add optional `merge-output` flag to guest-exec qapi

[Marc-André Lureau]

Hi

On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Currently, the captured output (via `capture-output`) is segregated into
> separate GuestExecStatus fields (`out-data` and `err-data`). This means
> that downstream consumers have no way to reassemble the captured data
> back into the original stream.
>
> This is relevant for chatty and semi-interactive (ie. read only) CLI
> tools.  Such tools may deliberately interleave stdout and stderr for
> visual effect. If segregated, the output becomes harder to visually
> understand.
>
> This commit adds a new optional flag to the guest-exec qapi to merge the
> output streams such that consumers can have a pristine view of the
> original command output.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  qga/commands.c       | 13 ++++++++++++-
>  qga/qapi-schema.json |  6 +++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 360077364e..14b970e768 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -274,6 +274,15 @@ static void guest_exec_child_watch(GPid pid, gint 
> status, gpointer data)
>  /** Reset ignored signals back to default. */
>  static void guest_exec_task_setup(gpointer data)
>  {
> +    bool has_merge = *(bool *)data;
> +
> +    if (has_merge) {
> +        if (dup2(STDOUT_FILENO, STDERR_FILENO) != 0) {
> +            slog("dup2() failed to merge stderr into stdout: %s",
> +                 strerror(errno));
> +        }
> +    }

https://docs.gtk.org/glib/callback.SpawnChildSetupFunc.html

"On Windows, the function is called in the parent. Its usefulness on
Windows is thus questionable. In many cases executing the child setup
function in the parent can have ill effects, and you should be very
careful when porting software to Windows that uses child setup
functions."

It looks like this would be bad.

> +
>  #if !defined(G_OS_WIN32)
>      struct sigaction sigact;
>
> @@ -385,6 +394,7 @@ GuestExec *qmp_guest_exec(const char *path,
>                         bool has_env, strList *env,
>                         const char *input_data,
>                         bool has_capture_output, bool capture_output,
> +                       bool has_merge_output, bool merge_output,
>                         Error **errp)
>  {
>      GPid pid;
> @@ -398,6 +408,7 @@ GuestExec *qmp_guest_exec(const char *path,
>      GIOChannel *in_ch, *out_ch, *err_ch;
>      GSpawnFlags flags;
>      bool has_output = (has_capture_output && capture_output);
> +    bool has_merge = (has_merge_output && merge_output);
>      g_autofree uint8_t *input = NULL;
>      size_t ninput = 0;
>
> @@ -421,7 +432,7 @@ GuestExec *qmp_guest_exec(const char *path,
>      }
>
>      ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> -            guest_exec_task_setup, NULL, &pid, input_data ? &in_fd : NULL,
> +            guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : 
> NULL,
>              has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
>      if (!ret) {
>          error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 796434ed34..4192fcc5a4 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1211,6 +1211,9 @@
>  # @input-data: data to be passed to process stdin (base64 encoded)
>  # @capture-output: bool flag to enable capture of
>  #                  stdout/stderr of running process. defaults to false.
> +# @merge-output: bool flag to merge stdout/stderr of running process
> +#                into stdout. only effective if used with @capture-output.
> +#                defaults to false.

Add (since: 8.0)

>  #
>  # Returns: PID on success.
>  #
> @@ -1218,7 +1221,8 @@
>  ##
>  { 'command': 'guest-exec',
>    'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> -               '*input-data': 'str', '*capture-output': 'bool' },
> +               '*input-data': 'str', '*capture-output': 'bool',
> +               '*merge-output': 'bool' },
>    'returns': 'GuestExec' }
>
>
> --
> 2.39.1
>
>


-- 
Marc-André Lureau