Re: [PATCH 2/3] qga: Add optional `merge-output` flag to guest-exec qapi
Hi
On Tue, Feb 28, 2023 at 5:15 AM Daniel Xu 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 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
> >> ---
> >> 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, , 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 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
Re: [PATCH 2/3] qga: Add optional `merge-output` flag to guest-exec qapi
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 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
>> ---
>> 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.
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, , 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 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
Re: [PATCH 2/3] qga: Add optional `merge-output` flag to guest-exec qapi
Hi
On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu 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
> ---
> 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, , 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 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
[PATCH 2/3] qga: Add optional `merge-output` flag to guest-exec qapi
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
---
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));
+}
+}
+
#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, , 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 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.
#
# 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
