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

[Marc-André Lureau]

Hi

On Tue, Feb 28, 2023 at 5:15 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Hi,
>
> On Mon, Feb 27, 2023, at 1:22 AM, Marc-André Lureau wrote:
> > 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.
>
> Ah that's a good catch. I'm not very familiar with windows APIs so
> unfortunately I don't have any good ideas here.
>
> Best I can tell g_spawn_async_with_pipes_and_fds() work with it's
> source_fds and target_fds mapping. But it looks like that came in
> glib 2.68 so we cannot use it yet.

g_spawn_async_with_fds() is from 2.58.. but we still depend on 2.56,
because of CentOS 8. And it seems we will have to wait until Dec 2023
to bump it.

I don't know whether it would be acceptable to simply return an error
when using 'merge-output' on old host (with glib < 2.58).

 Otherwise, I think you should use g_spawn_async_with_fds() when
possible, and use the ChildSetupFunc fallback, but only on Unix (for
CentOS 8!).

>
> How about limiting this merge-output flag to linux/unix systems
> for now? Could document this in the qapi doc string.
>
> >
> >> +
> >>  #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)
>
> Ack.
>
> [...]
>
> Thanks,
> Daniel



-- 
Marc-André Lureau