Re: [PATCH v5 2/3] qga: Add `merged` variant to GuestExecCaptureOutputMode
Hi Daniel,
Sorry about the delay -- was out of town the past week.
On Fri, Mar 10, 2023, at 2:24 AM, Daniel P. Berrangé wrote:
> On Thu, Mar 09, 2023 at 03:40:57PM -0700, Daniel Xu wrote:
>> Currently, any 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 enum variant to the GuestExecCaptureOutputMode
>> qapi to merge the output streams such that consumers can have a pristine
>> view of the original command output.
>>
>> Signed-off-by: Daniel Xu
>> ---
>> qga/commands.c | 31 +--
>> qga/qapi-schema.json | 4 +++-
>> 2 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/qga/commands.c b/qga/commands.c
>> index 01f68b45ab..c347d434ed 100644
>> --- a/qga/commands.c
>> +++ b/qga/commands.c
>> @@ -270,12 +270,26 @@ 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) {
>> +/*
>> + * FIXME: When `GLIB_VERSION_MIN_REQUIRED` is bumped to 2.58+, use
>> + * g_spawn_async_with_fds() to be portable on windows. The current
>> + * logic does not work on windows b/c `GSpawnChildSetupFunc` is run
>> + * inside the parent, not the child.
>> + */
>> +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(, 0, sizeof(struct sigaction));
>> sigact.sa_handler = SIG_DFL;
>>
>> @@ -409,6 +423,7 @@ GuestExec *qmp_guest_exec(const char *path,
>> GIOChannel *in_ch, *out_ch, *err_ch;
>> GSpawnFlags flags;
>> bool has_output = false;
>> +bool has_merge = false;
>
> Wrap in #ifndef _WIN32
I think it would be better to leave this variable un-gated b/c gating it
would make the later call to g_spawn_async_with_pipes() less clean.
I don't think it'll trigger any unused variable warnings either since we
are technically always using it.
>
>> GuestExecCaptureOutputMode output_mode;
>> g_autofree uint8_t *input = NULL;
>> size_t ninput = 0;
>> @@ -445,13 +460,25 @@ GuestExec *qmp_guest_exec(const char *path,
>> case GUEST_EXEC_CAPTURE_OUTPUT_MODE_SEPARATED:
>> has_output = true;
>> break;
>> +case GUEST_EXEC_CAPTURE_OUTPUT_MODE_MERGED:
>> +has_output = true;
>> +has_merge = true;
>> +break;
>
> Wrap in #ifndef _WIN32
>
>> case GUEST_EXEC_CAPTURE_OUTPUT_MODE__MAX:
>> /* Silence warning; impossible branch */
>> break;
>> }
>>
>> +#if defined(G_OS_WIN32)
>> +/* FIXME: see comment in guest_exec_task_setup() */
>> +if (has_merge) {
>> +error_setg(errp, "merged unsupported on windows");
>> +return NULL;
>> +}
>> +#endif
>
> THis can be dropped, since 'has_merge' won't exist for
> Win32 builds.
>
>> +
>> ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
>> -guest_exec_task_setup, NULL, , input_data ? _fd : NULL,
>> +guest_exec_task_setup, _merge, , input_data ? _fd :
>> NULL,
>> has_output ? _fd : NULL, has_output ? _fd : NULL,
>> );
>> if (!ret) {
>> error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index d1e00a4234..b4782525ae 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -1210,11 +1210,13 @@
>> # @stderr: only capture stderr
>> # @separated: capture both stdout and stderr, but separated into
>> # GuestExecStatus out-data and err-data, respectively
>> +# @merged: capture both stdout and stderr, but merge together
>> +# into out-data. not effective on windows guests.
>> #
>> # Since: 8.0
>> ##
>> { 'enum': 'GuestExecCaptureOutputMode',
>> - 'data': [ 'none', 'stdout', 'stderr', 'separated' ] }
>> + 'data': [ 'none', 'stdout', 'stderr', 'separated', 'merged' ] }
>
> Actually, I've just realized we can make this conditional:
>
>
> 'data': [ 'none', 'stdout', 'stderr', 'separated',
>{ 'name': 'merged', 'if': 'CONFIG_WIN32' } ] }
>
> so the constant doesn't even exist in Win32 builds.
Ack, that looks cleaner.
[...]
Thanks,
Daniel
Re: [PATCH v5 2/3] qga: Add `merged` variant to GuestExecCaptureOutputMode
On Thu, Mar 09, 2023 at 03:40:57PM -0700, Daniel Xu wrote:
> Currently, any 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 enum variant to the GuestExecCaptureOutputMode
> qapi to merge the output streams such that consumers can have a pristine
> view of the original command output.
>
> Signed-off-by: Daniel Xu
> ---
> qga/commands.c | 31 +--
> qga/qapi-schema.json | 4 +++-
> 2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 01f68b45ab..c347d434ed 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -270,12 +270,26 @@ 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) {
> +/*
> + * FIXME: When `GLIB_VERSION_MIN_REQUIRED` is bumped to 2.58+, use
> + * g_spawn_async_with_fds() to be portable on windows. The current
> + * logic does not work on windows b/c `GSpawnChildSetupFunc` is run
> + * inside the parent, not the child.
> + */
> +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(, 0, sizeof(struct sigaction));
> sigact.sa_handler = SIG_DFL;
>
> @@ -409,6 +423,7 @@ GuestExec *qmp_guest_exec(const char *path,
> GIOChannel *in_ch, *out_ch, *err_ch;
> GSpawnFlags flags;
> bool has_output = false;
> +bool has_merge = false;
Wrap in #ifndef _WIN32
> GuestExecCaptureOutputMode output_mode;
> g_autofree uint8_t *input = NULL;
> size_t ninput = 0;
> @@ -445,13 +460,25 @@ GuestExec *qmp_guest_exec(const char *path,
> case GUEST_EXEC_CAPTURE_OUTPUT_MODE_SEPARATED:
> has_output = true;
> break;
> +case GUEST_EXEC_CAPTURE_OUTPUT_MODE_MERGED:
> +has_output = true;
> +has_merge = true;
> +break;
Wrap in #ifndef _WIN32
> case GUEST_EXEC_CAPTURE_OUTPUT_MODE__MAX:
> /* Silence warning; impossible branch */
> break;
> }
>
> +#if defined(G_OS_WIN32)
> +/* FIXME: see comment in guest_exec_task_setup() */
> +if (has_merge) {
> +error_setg(errp, "merged unsupported on windows");
> +return NULL;
> +}
> +#endif
THis can be dropped, since 'has_merge' won't exist for
Win32 builds.
> +
> ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> -guest_exec_task_setup, NULL, , input_data ? _fd : NULL,
> +guest_exec_task_setup, _merge, , input_data ? _fd :
> NULL,
> has_output ? _fd : NULL, has_output ? _fd : NULL, );
> if (!ret) {
> error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index d1e00a4234..b4782525ae 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1210,11 +1210,13 @@
> # @stderr: only capture stderr
> # @separated: capture both stdout and stderr, but separated into
> # GuestExecStatus out-data and err-data, respectively
> +# @merged: capture both stdout and stderr, but merge together
> +# into out-data. not effective on windows guests.
> #
> # Since: 8.0
> ##
> { 'enum': 'GuestExecCaptureOutputMode',
> - 'data': [ 'none', 'stdout', 'stderr', 'separated' ] }
> + 'data': [ 'none', 'stdout', 'stderr', 'separated', 'merged' ] }
Actually, I've just realized we can make this conditional:
'data': [ 'none', 'stdout', 'stderr', 'separated',
{ 'name': 'merged', 'if': 'CONFIG_WIN32' } ] }
so the constant doesn't even exist in Win32 builds.
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 :|
Re: [PATCH v5 2/3] qga: Add `merged` variant to GuestExecCaptureOutputMode
On Thu, Mar 09, 2023 at 03:40:57PM -0700, Daniel Xu wrote: > Currently, any 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 enum variant to the GuestExecCaptureOutputMode > qapi to merge the output streams such that consumers can have a pristine > view of the original command output. > > Signed-off-by: Daniel Xu > --- > qga/commands.c | 31 +-- > qga/qapi-schema.json | 4 +++- > 2 files changed, 32 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé 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 :|
[PATCH v5 2/3] qga: Add `merged` variant to GuestExecCaptureOutputMode
Currently, any 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 enum variant to the GuestExecCaptureOutputMode
qapi to merge the output streams such that consumers can have a pristine
view of the original command output.
Signed-off-by: Daniel Xu
---
qga/commands.c | 31 +--
qga/qapi-schema.json | 4 +++-
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/qga/commands.c b/qga/commands.c
index 01f68b45ab..c347d434ed 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -270,12 +270,26 @@ 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) {
+/*
+ * FIXME: When `GLIB_VERSION_MIN_REQUIRED` is bumped to 2.58+, use
+ * g_spawn_async_with_fds() to be portable on windows. The current
+ * logic does not work on windows b/c `GSpawnChildSetupFunc` is run
+ * inside the parent, not the child.
+ */
+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(, 0, sizeof(struct sigaction));
sigact.sa_handler = SIG_DFL;
@@ -409,6 +423,7 @@ GuestExec *qmp_guest_exec(const char *path,
GIOChannel *in_ch, *out_ch, *err_ch;
GSpawnFlags flags;
bool has_output = false;
+bool has_merge = false;
GuestExecCaptureOutputMode output_mode;
g_autofree uint8_t *input = NULL;
size_t ninput = 0;
@@ -445,13 +460,25 @@ GuestExec *qmp_guest_exec(const char *path,
case GUEST_EXEC_CAPTURE_OUTPUT_MODE_SEPARATED:
has_output = true;
break;
+case GUEST_EXEC_CAPTURE_OUTPUT_MODE_MERGED:
+has_output = true;
+has_merge = true;
+break;
case GUEST_EXEC_CAPTURE_OUTPUT_MODE__MAX:
/* Silence warning; impossible branch */
break;
}
+#if defined(G_OS_WIN32)
+/* FIXME: see comment in guest_exec_task_setup() */
+if (has_merge) {
+error_setg(errp, "merged unsupported on windows");
+return NULL;
+}
+#endif
+
ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
-guest_exec_task_setup, NULL, , input_data ? _fd : NULL,
+guest_exec_task_setup, _merge, , input_data ? _fd :
NULL,
has_output ? _fd : NULL, has_output ? _fd : NULL, );
if (!ret) {
error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index d1e00a4234..b4782525ae 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1210,11 +1210,13 @@
# @stderr: only capture stderr
# @separated: capture both stdout and stderr, but separated into
# GuestExecStatus out-data and err-data, respectively
+# @merged: capture both stdout and stderr, but merge together
+# into out-data. not effective on windows guests.
#
# Since: 8.0
##
{ 'enum': 'GuestExecCaptureOutputMode',
- 'data': [ 'none', 'stdout', 'stderr', 'separated' ] }
+ 'data': [ 'none', 'stdout', 'stderr', 'separated', 'merged' ] }
##
# @GuestExecCaptureOutput:
--
2.39.1
