Message ID | e4a6d274d554479c665de82b3627f1df2055306a.1677609866.git.dxu@dxuuu.xyz |
---|---|
State | New |
Headers | show |
Series | qga: Add optional `merge-output` flag to guest-exec QAPI | expand |
Hi On Tue, Feb 28, 2023 at 10:51 PM 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 | 14 ++++++++++++-- > qga/qapi-schema.json | 6 +++++- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/qga/commands.c b/qga/commands.c > index 172826f8f8..e13a8e86df 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -270,12 +270,20 @@ static void guest_exec_child_watch(GPid pid, gint status, gpointer data) > g_spawn_close_pid(pid); > } > > -/** Reset ignored signals back to default. */ > static void guest_exec_task_setup(gpointer data) > { > #if !defined(G_OS_WIN32) > + bool has_merge = *(bool *)data; > struct sigaction sigact; > > + if (has_merge) { > + if (dup2(STDOUT_FILENO, STDERR_FILENO) != 0) { > + slog("dup2() failed to merge stderr into stdout: %s", > + strerror(errno)); I would leave a FIXME comment for glib 2.58 g_spawn_async_with_fds() usage > + } > + } > + > + /* Reset ignored signals back to default. */ > memset(&sigact, 0, sizeof(struct sigaction)); > sigact.sa_handler = SIG_DFL; > > @@ -384,6 +392,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; > @@ -397,6 +406,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; > > @@ -420,7 +430,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..9c2367acdf 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. > +# not effective on windows guests. defaults to false. (since 8.0) I think you should return an error on Windows instead. > # > # 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 >
Hi Marc-André, Apologies if it looks like I ignored your previous email. I forgot to renew my domain registration so all my emails were getting blackholed or bounced for ~17 hours. But w.r.t. fallback with old versions of glib, I think it may be a little uglier than that. It looks like the build system uses preprocessor macros to warn on too-new APIs. So I suppose we could #undef and redefine GLIB_VERSION_MIN_REQUIRED, but may not be very nice. But if you feel strongly I can try that. On Tue, Feb 28, 2023 at 11:26:09PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Feb 28, 2023 at 10:51 PM 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 | 14 ++++++++++++-- > > qga/qapi-schema.json | 6 +++++- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/qga/commands.c b/qga/commands.c > > index 172826f8f8..e13a8e86df 100644 > > --- a/qga/commands.c > > +++ b/qga/commands.c > > @@ -270,12 +270,20 @@ static void guest_exec_child_watch(GPid pid, gint status, gpointer data) > > g_spawn_close_pid(pid); > > } > > > > -/** Reset ignored signals back to default. */ > > static void guest_exec_task_setup(gpointer data) > > { > > #if !defined(G_OS_WIN32) > > + bool has_merge = *(bool *)data; > > struct sigaction sigact; > > > > + if (has_merge) { > > + if (dup2(STDOUT_FILENO, STDERR_FILENO) != 0) { > > + slog("dup2() failed to merge stderr into stdout: %s", > > + strerror(errno)); > > I would leave a FIXME comment for glib 2.58 g_spawn_async_with_fds() usage Ack. > > > + } > > + } > > + > > + /* Reset ignored signals back to default. */ > > memset(&sigact, 0, sizeof(struct sigaction)); > > sigact.sa_handler = SIG_DFL; > > > > @@ -384,6 +392,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; > > @@ -397,6 +406,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; > > > > @@ -420,7 +430,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..9c2367acdf 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. > > +# not effective on windows guests. defaults to false. (since 8.0) > > I think you should return an error on Windows instead. Ack. [...] Thanks, Daniel
diff --git a/qga/commands.c b/qga/commands.c index 172826f8f8..e13a8e86df 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -270,12 +270,20 @@ static void guest_exec_child_watch(GPid pid, gint status, gpointer data) g_spawn_close_pid(pid); } -/** Reset ignored signals back to default. */ static void guest_exec_task_setup(gpointer data) { #if !defined(G_OS_WIN32) + bool has_merge = *(bool *)data; struct sigaction sigact; + if (has_merge) { + if (dup2(STDOUT_FILENO, STDERR_FILENO) != 0) { + slog("dup2() failed to merge stderr into stdout: %s", + strerror(errno)); + } + } + + /* Reset ignored signals back to default. */ memset(&sigact, 0, sizeof(struct sigaction)); sigact.sa_handler = SIG_DFL; @@ -384,6 +392,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; @@ -397,6 +406,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; @@ -420,7 +430,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..9c2367acdf 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. +# not effective on windows guests. defaults to false. (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' }
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 | 14 ++++++++++++-- qga/qapi-schema.json | 6 +++++- 2 files changed, 17 insertions(+), 3 deletions(-)