[PATCH 0/3] qga: Add optional `merge-output` flag to guest-exec QAPI

[PATCH 0/3] qga: Add optional `merge-output` flag to guest-exec QAPI

[Daniel Xu]

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 patchset adds support for merging stderr and stdout output streams
via a new QAPI flag.

Daniel Xu (3):
  qga: test: Use absolute path to test data
  qga: Add optional `merge-output` flag to guest-exec qapi
  qga: test: Add tests for `merge-output` flag

 qga/commands.c        |  13 +++-
 qga/qapi-schema.json  |   6 +-
 tests/unit/test-qga.c | 135 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 133 insertions(+), 21 deletions(-)

-- 
2.39.1

[PATCH 1/3] qga: test: Use absolute path to test data

[Daniel Xu]

It looks like qga's working directory is in a tempdir. So the relative
path that the test case gives qga through the QGA_OS_RELEASE=
env variable does not resolve correctly.

Fix by doing a poor man's path canonicalization of the test data file.

Note we cannot use g_canonicalize_filename() b/c that helper was only
introduced in glib 2.58 and the current GLIB_VERSION_MAX_ALLOWED is
pinned to 2.56.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index b4e0a14573..9d8e1d1cd3 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -881,13 +881,16 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
     const gchar *str;
+    g_autofree const gchar *cwd;
     g_autoptr(QDict) ret = NULL;
     char *env[2];
     QDict *val;
 
+    cwd = g_get_current_dir();
     env[0] = g_strdup_printf(
-        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
-        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+        "QGA_OS_RELEASE=%s%c%s%c..%cdata%ctest-qga-os-release",
+        cwd, G_DIR_SEPARATOR, g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR,
+        G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
     fixture_setup(&fixture, NULL, env);
 
-- 
2.39.1

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

[Daniel Xu]

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));
+        }
+    }
+
 #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.
 #
 # 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

[PATCH 3/3] qga: test: Add tests for `merge-output` flag

[Daniel Xu]

This commit adds a test to ensure `merge-output` functions as expected.
We also add a negative test to ensure we haven't regressed previous
functionality.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 9d8e1d1cd3..0b3966024c 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -755,6 +755,31 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
     g_assert_cmpstr(status, ==, "thawed");
 }
 
+static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
+{
+    QDict *ret = NULL;
+    int64_t now;
+    bool exited;
+    QDict *val;
+
+    now = g_get_monotonic_time();
+    do {
+        ret = qmp_fd(fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
+        g_assert_nonnull(ret);
+        val = qdict_get_qdict(ret, "return");
+        exited = qdict_get_bool(val, "exited");
+        if (!exited) {
+            qobject_unref(ret);
+        }
+    } while (!exited &&
+             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
+    g_assert(exited);
+
+    return ret;
+}
+
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -762,9 +787,8 @@ static void test_qga_guest_exec(gconstpointer fix)
     QDict *val;
     const gchar *out;
     g_autofree guchar *decoded = NULL;
-    int64_t pid, now, exitcode;
+    int64_t pid, exitcode;
     gsize len;
-    bool exited;
 
     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -777,21 +801,7 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpint(pid, >, 0);
     qobject_unref(ret);
 
-    /* wait for completion */
-    now = g_get_monotonic_time();
-    do {
-        ret = qmp_fd(fixture->fd,
-                     "{'execute': 'guest-exec-status',"
-                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
-        g_assert_nonnull(ret);
-        val = qdict_get_qdict(ret, "return");
-        exited = qdict_get_bool(val, "exited");
-        if (!exited) {
-            qobject_unref(ret);
-        }
-    } while (!exited &&
-             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
-    g_assert(exited);
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
 
     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
@@ -802,6 +812,86 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
 }
 
+static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *out_decoded = NULL;
+    g_autofree guchar *err_decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    out_decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    err_decoded = g_base64_decode(err, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
+}
+
+static void test_qga_guest_exec_output_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true,"
+                 " 'merge-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 28);
+    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    g_assert_null(err);
+}
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -975,6 +1065,10 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
     g_test_add_data_func("/qga/config", NULL, test_qga_config);
     g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
+    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
+                         test_qga_guest_exec_output_no_merge);
+    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
+                         test_qga_guest_exec_output_merge);
     g_test_add_data_func("/qga/guest-exec-invalid", &fix,
                          test_qga_guest_exec_invalid);
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,
-- 
2.39.1

[PATCH 0/3] qga: Add optional `merge-output` flag to guest-exec QAPI

[Daniel Xu]

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 patchset adds support for merging stderr and stdout output streams
via a new QAPI flag.

Daniel Xu (3):
  qga: test: Use absolute path to test data
  qga: Add optional `merge-output` flag to guest-exec qapi
  qga: test: Add tests for `merge-output` flag

 qga/commands.c        |  13 +++-
 qga/qapi-schema.json  |   6 +-
 tests/unit/test-qga.c | 135 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 133 insertions(+), 21 deletions(-)

-- 
2.39.1

[PATCH 1/3] qga: test: Use absolute path to test data

[Daniel Xu]

It looks like qga's working directory is in a tempdir. So the relative
path that the test case gives qga through the QGA_OS_RELEASE=
env variable does not resolve correctly.

Fix by doing a poor man's path canonicalization of the test data file.

Note we cannot use g_canonicalize_filename() b/c that helper was only
introduced in glib 2.58 and the current GLIB_VERSION_MAX_ALLOWED is
pinned to 2.56.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index b4e0a14573..9d8e1d1cd3 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -881,13 +881,16 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
     const gchar *str;
+    g_autofree const gchar *cwd;
     g_autoptr(QDict) ret = NULL;
     char *env[2];
     QDict *val;
 
+    cwd = g_get_current_dir();
     env[0] = g_strdup_printf(
-        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
-        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+        "QGA_OS_RELEASE=%s%c%s%c..%cdata%ctest-qga-os-release",
+        cwd, G_DIR_SEPARATOR, g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR,
+        G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
     fixture_setup(&fixture, NULL, env);
 
-- 
2.39.1

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

[Daniel Xu]

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));
+        }
+    }
+
 #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.
 #
 # 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

[PATCH 3/3] qga: test: Add tests for `merge-output` flag

[Daniel Xu]

This commit adds a test to ensure `merge-output` functions as expected.
We also add a negative test to ensure we haven't regressed previous
functionality.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 9d8e1d1cd3..0b3966024c 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -755,6 +755,31 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
     g_assert_cmpstr(status, ==, "thawed");
 }
 
+static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
+{
+    QDict *ret = NULL;
+    int64_t now;
+    bool exited;
+    QDict *val;
+
+    now = g_get_monotonic_time();
+    do {
+        ret = qmp_fd(fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
+        g_assert_nonnull(ret);
+        val = qdict_get_qdict(ret, "return");
+        exited = qdict_get_bool(val, "exited");
+        if (!exited) {
+            qobject_unref(ret);
+        }
+    } while (!exited &&
+             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
+    g_assert(exited);
+
+    return ret;
+}
+
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -762,9 +787,8 @@ static void test_qga_guest_exec(gconstpointer fix)
     QDict *val;
     const gchar *out;
     g_autofree guchar *decoded = NULL;
-    int64_t pid, now, exitcode;
+    int64_t pid, exitcode;
     gsize len;
-    bool exited;
 
     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -777,21 +801,7 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpint(pid, >, 0);
     qobject_unref(ret);
 
-    /* wait for completion */
-    now = g_get_monotonic_time();
-    do {
-        ret = qmp_fd(fixture->fd,
-                     "{'execute': 'guest-exec-status',"
-                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
-        g_assert_nonnull(ret);
-        val = qdict_get_qdict(ret, "return");
-        exited = qdict_get_bool(val, "exited");
-        if (!exited) {
-            qobject_unref(ret);
-        }
-    } while (!exited &&
-             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
-    g_assert(exited);
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
 
     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
@@ -802,6 +812,86 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
 }
 
+static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *out_decoded = NULL;
+    g_autofree guchar *err_decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    out_decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    err_decoded = g_base64_decode(err, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
+}
+
+static void test_qga_guest_exec_output_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true,"
+                 " 'merge-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 28);
+    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    g_assert_null(err);
+}
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -975,6 +1065,10 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
     g_test_add_data_func("/qga/config", NULL, test_qga_config);
     g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
+    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
+                         test_qga_guest_exec_output_no_merge);
+    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
+                         test_qga_guest_exec_output_merge);
     g_test_add_data_func("/qga/guest-exec-invalid", &fix,
                          test_qga_guest_exec_invalid);
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,
-- 
2.39.1

[PATCH 0/3] qga: Add optional `merge-output` flag to guest-exec QAPI

[Daniel Xu]

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 patchset adds support for merging stderr and stdout output streams
via a new QAPI flag.

Daniel Xu (3):
  qga: test: Use absolute path to test data
  qga: Add optional `merge-output` flag to guest-exec qapi
  qga: test: Add tests for `merge-output` flag

 qga/commands.c        |  13 +++-
 qga/qapi-schema.json  |   6 +-
 tests/unit/test-qga.c | 135 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 133 insertions(+), 21 deletions(-)

-- 
2.39.1

[PATCH 1/3] qga: test: Use absolute path to test data

[Daniel Xu]

It looks like qga's working directory is in a tempdir. So the relative
path that the test case gives qga through the QGA_OS_RELEASE=
env variable does not resolve correctly.

Fix by doing a poor man's path canonicalization of the test data file.

Note we cannot use g_canonicalize_filename() b/c that helper was only
introduced in glib 2.58 and the current GLIB_VERSION_MAX_ALLOWED is
pinned to 2.56.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index b4e0a14573..9d8e1d1cd3 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -881,13 +881,16 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
     const gchar *str;
+    g_autofree const gchar *cwd;
     g_autoptr(QDict) ret = NULL;
     char *env[2];
     QDict *val;
 
+    cwd = g_get_current_dir();
     env[0] = g_strdup_printf(
-        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
-        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+        "QGA_OS_RELEASE=%s%c%s%c..%cdata%ctest-qga-os-release",
+        cwd, G_DIR_SEPARATOR, g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR,
+        G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
     fixture_setup(&fixture, NULL, env);
 
-- 
2.39.1

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

[Daniel Xu]

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));
+        }
+    }
+
 #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.
 #
 # 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

[PATCH 3/3] qga: test: Add tests for `merge-output` flag

[Daniel Xu]

This commit adds a test to ensure `merge-output` functions as expected.
We also add a negative test to ensure we haven't regressed previous
functionality.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 9d8e1d1cd3..0b3966024c 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -755,6 +755,31 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
     g_assert_cmpstr(status, ==, "thawed");
 }
 
+static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
+{
+    QDict *ret = NULL;
+    int64_t now;
+    bool exited;
+    QDict *val;
+
+    now = g_get_monotonic_time();
+    do {
+        ret = qmp_fd(fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
+        g_assert_nonnull(ret);
+        val = qdict_get_qdict(ret, "return");
+        exited = qdict_get_bool(val, "exited");
+        if (!exited) {
+            qobject_unref(ret);
+        }
+    } while (!exited &&
+             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
+    g_assert(exited);
+
+    return ret;
+}
+
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -762,9 +787,8 @@ static void test_qga_guest_exec(gconstpointer fix)
     QDict *val;
     const gchar *out;
     g_autofree guchar *decoded = NULL;
-    int64_t pid, now, exitcode;
+    int64_t pid, exitcode;
     gsize len;
-    bool exited;
 
     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -777,21 +801,7 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpint(pid, >, 0);
     qobject_unref(ret);
 
-    /* wait for completion */
-    now = g_get_monotonic_time();
-    do {
-        ret = qmp_fd(fixture->fd,
-                     "{'execute': 'guest-exec-status',"
-                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
-        g_assert_nonnull(ret);
-        val = qdict_get_qdict(ret, "return");
-        exited = qdict_get_bool(val, "exited");
-        if (!exited) {
-            qobject_unref(ret);
-        }
-    } while (!exited &&
-             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
-    g_assert(exited);
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
 
     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
@@ -802,6 +812,86 @@ static void test_qga_guest_exec(gconstpointer fix)
     g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
 }
 
+static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *out_decoded = NULL;
+    g_autofree guchar *err_decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    out_decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    err_decoded = g_base64_decode(err, &len);
+    g_assert_cmpint(len, ==, 14);
+    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
+}
+
+static void test_qga_guest_exec_output_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *out, *err;
+    g_autofree guchar *decoded = NULL;
+    int64_t pid, exitcode;
+    gsize len;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
+                 " 'capture-output': true,"
+                 " 'merge-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    qobject_unref(ret);
+
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+
+    /* check stdout */
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 28);
+    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
+
+    /* check stderr */
+    err = qdict_get_try_str(val, "err-data");
+    g_assert_null(err);
+}
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -975,6 +1065,10 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
     g_test_add_data_func("/qga/config", NULL, test_qga_config);
     g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
+    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
+                         test_qga_guest_exec_output_no_merge);
+    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
+                         test_qga_guest_exec_output_merge);
     g_test_add_data_func("/qga/guest-exec-invalid", &fix,
                          test_qga_guest_exec_invalid);
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,
-- 
2.39.1

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

[Daniel Xu]

On Thu, Feb 23, 2023, at 7:05 PM, 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 patchset adds support for merging stderr and stdout output streams
> via a new QAPI flag.
>
> Daniel Xu (3):
>   qga: test: Use absolute path to test data
>   qga: Add optional `merge-output` flag to guest-exec qapi
>   qga: test: Add tests for `merge-output` flag
>
>  qga/commands.c        |  13 +++-
>  qga/qapi-schema.json  |   6 +-
>  tests/unit/test-qga.c | 135 ++++++++++++++++++++++++++++++++++++------
>  3 files changed, 133 insertions(+), 21 deletions(-)
>
> -- 
> 2.39.1

Apologies for spamming the list. I thought my mail provider
was swallowing my sends but it looks like the list was a bit
slow.

Thanks,
Daniel

Re: [PATCH 1/3] qga: test: Use absolute path to test data

[Marc-André Lureau]

Hi

On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> It looks like qga's working directory is in a tempdir. So the relative
> path that the test case gives qga through the QGA_OS_RELEASE=
> env variable does not resolve correctly.
>
> Fix by doing a poor man's path canonicalization of the test data file.
>
> Note we cannot use g_canonicalize_filename() b/c that helper was only
> introduced in glib 2.58 and the current GLIB_VERSION_MAX_ALLOWED is
> pinned to 2.56.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

This breaks "meson test test-qga" for me. How do you run the tests?

> ---
>  tests/unit/test-qga.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index b4e0a14573..9d8e1d1cd3 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -881,13 +881,16 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
>  {
>      TestFixture fixture;
>      const gchar *str;
> +    g_autofree const gchar *cwd;

const is not appropriate here, interesting I don't get a warning

>      g_autoptr(QDict) ret = NULL;
>      char *env[2];
>      QDict *val;
>
> +    cwd = g_get_current_dir();
>      env[0] = g_strdup_printf(
> -        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
> -        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> +        "QGA_OS_RELEASE=%s%c%s%c..%cdata%ctest-qga-os-release",
> +        cwd, G_DIR_SEPARATOR, g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR,
> +        G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>      env[1] = NULL;
>      fixture_setup(&fixture, NULL, env);
>
> --
> 2.39.1
>
>


-- 
Marc-André Lureau

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

Re: [PATCH 3/3] qga: test: Add tests for `merge-output` flag

[Marc-André Lureau]

Hi

On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> This commit adds a test to ensure `merge-output` functions as expected.
> We also add a negative test to ensure we haven't regressed previous
> functionality.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Please check your patch with ASAN, you have use after-free introduced
by this change:
==664972==ERROR: AddressSanitizer: heap-use-after-free on address
0x621000135028 at pc 0x55e617a38b39 bp 0x7fff7fe85390 sp
0x7fff7fe85388
READ of size 8 at 0x621000135028 thread T0
    #0 0x55e617a38b38 in qdict_find ../qobject/qdict.c:96
    #1 0x55e617a39bea in qdict_get ../qobject/qdict.c:164
    #2 0x55e617a39bea in qdict_get_int ../qobject/qdict.c:209
    #3 0x55e6179e2519 in test_qga_guest_exec ../tests/unit/test-qga.c:807
    #4 0x7fbaa499dc7d in g_test_run_suite_internal
(/lib64/libglib-2.0.so.0+0x84c7d)
    #5 0x7fbaa499d9e4 in g_test_run_suite_internal
(/lib64/libglib-2.0.so.0+0x849e4)
    #6 0x7fbaa499e181 in g_test_run_suite (/lib64/libglib-2.0.so.0+0x85181)
    #7 0x7fbaa49966ec in g_test_run (/lib64/libglib-2.0.so.0+0x7d6ec)
    #8 0x55e6179da0ac in main ../tests/unit/test-qga.c:1083
    #9 0x7fbaa384a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #10 0x7fbaa384a5c8 in __libc_start_main@GLIBC_2.2.5
(/lib64/libc.so.6+0x275c8)
    #11 0x55e6179daf44 in _start
(/home/elmarco/src/qemu/build/tests/unit/test-qga+0x1bbf44)

thanks
> ---
>  tests/unit/test-qga.c | 128 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 111 insertions(+), 17 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index 9d8e1d1cd3..0b3966024c 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -755,6 +755,31 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
>      g_assert_cmpstr(status, ==, "thawed");
>  }
>
> +static QDict *wait_for_guest_exec_completion(int fd, int64_t pid)
> +{
> +    QDict *ret = NULL;
> +    int64_t now;
> +    bool exited;
> +    QDict *val;
> +
> +    now = g_get_monotonic_time();
> +    do {
> +        ret = qmp_fd(fd,
> +                     "{'execute': 'guest-exec-status',"
> +                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
> +        g_assert_nonnull(ret);
> +        val = qdict_get_qdict(ret, "return");
> +        exited = qdict_get_bool(val, "exited");
> +        if (!exited) {
> +            qobject_unref(ret);
> +        }
> +    } while (!exited &&
> +             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> +    g_assert(exited);
> +
> +    return ret;
> +}
> +
>  static void test_qga_guest_exec(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> @@ -762,9 +787,8 @@ static void test_qga_guest_exec(gconstpointer fix)
>      QDict *val;
>      const gchar *out;
>      g_autofree guchar *decoded = NULL;
> -    int64_t pid, now, exitcode;
> +    int64_t pid, exitcode;
>      gsize len;
> -    bool exited;
>
>      /* exec 'echo foo bar' */
>      ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> @@ -777,21 +801,7 @@ static void test_qga_guest_exec(gconstpointer fix)
>      g_assert_cmpint(pid, >, 0);
>      qobject_unref(ret);
>
> -    /* wait for completion */
> -    now = g_get_monotonic_time();
> -    do {
> -        ret = qmp_fd(fixture->fd,
> -                     "{'execute': 'guest-exec-status',"
> -                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
> -        g_assert_nonnull(ret);
> -        val = qdict_get_qdict(ret, "return");
> -        exited = qdict_get_bool(val, "exited");
> -        if (!exited) {
> -            qobject_unref(ret);
> -        }
> -    } while (!exited &&
> -             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> -    g_assert(exited);
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
>
>      /* check stdout */
>      exitcode = qdict_get_int(val, "exitcode");
> @@ -802,6 +812,86 @@ static void test_qga_guest_exec(gconstpointer fix)
>      g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
>  }
>
> +static void test_qga_guest_exec_output_no_merge(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    g_autoptr(QDict) ret = NULL;
> +    QDict *val;
> +    const gchar *out, *err;
> +    g_autofree guchar *out_decoded = NULL;
> +    g_autofree guchar *err_decoded = NULL;
> +    int64_t pid, exitcode;
> +    gsize len;
> +
> +    /* exec 'echo foo bar' */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/bash',"
> +                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
> +                 " 'capture-output': true } }");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +    val = qdict_get_qdict(ret, "return");
> +    pid = qdict_get_int(val, "pid");
> +    g_assert_cmpint(pid, >, 0);
> +    qobject_unref(ret);
> +
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
> +
> +    exitcode = qdict_get_int(val, "exitcode");
> +    g_assert_cmpint(exitcode, ==, 0);
> +
> +    /* check stdout */
> +    out = qdict_get_str(val, "out-data");
> +    out_decoded = g_base64_decode(out, &len);
> +    g_assert_cmpint(len, ==, 14);
> +    g_assert_cmpstr((char *)out_decoded, ==, "stdout\nstdout\n");
> +
> +    /* check stderr */
> +    err = qdict_get_try_str(val, "err-data");
> +    err_decoded = g_base64_decode(err, &len);
> +    g_assert_cmpint(len, ==, 14);
> +    g_assert_cmpstr((char *)err_decoded, ==, "stderr\nstderr\n");
> +}
> +
> +static void test_qga_guest_exec_output_merge(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    g_autoptr(QDict) ret = NULL;
> +    QDict *val;
> +    const gchar *out, *err;
> +    g_autofree guchar *decoded = NULL;
> +    int64_t pid, exitcode;
> +    gsize len;
> +
> +    /* exec 'echo foo bar' */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/bash',"
> +                 " 'arg': [ '-c', 'for i in $(seq 4); do if (( $i %% 2 )); then echo stdout; else echo stderr 1>&2; fi; done;' ],"
> +                 " 'capture-output': true,"
> +                 " 'merge-output': true } }");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +    val = qdict_get_qdict(ret, "return");
> +    pid = qdict_get_int(val, "pid");
> +    g_assert_cmpint(pid, >, 0);
> +    qobject_unref(ret);
> +
> +    ret = wait_for_guest_exec_completion(fixture->fd, pid);
> +
> +    exitcode = qdict_get_int(val, "exitcode");
> +    g_assert_cmpint(exitcode, ==, 0);
> +
> +    /* check stdout */
> +    out = qdict_get_str(val, "out-data");
> +    decoded = g_base64_decode(out, &len);
> +    g_assert_cmpint(len, ==, 28);
> +    g_assert_cmpstr((char *)decoded, ==, "stdout\nstderr\nstdout\nstderr\n");
> +
> +    /* check stderr */
> +    err = qdict_get_try_str(val, "err-data");
> +    g_assert_null(err);
> +}
> +
>  static void test_qga_guest_exec_invalid(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> @@ -975,6 +1065,10 @@ int main(int argc, char **argv)
>      g_test_add_data_func("/qga/blockedrpcs", NULL, test_qga_blockedrpcs);
>      g_test_add_data_func("/qga/config", NULL, test_qga_config);
>      g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
> +    g_test_add_data_func("/qga/guest-exec-output-no-merge", &fix,
> +                         test_qga_guest_exec_output_no_merge);
> +    g_test_add_data_func("/qga/guest-exec-output-merge", &fix,
> +                         test_qga_guest_exec_output_merge);
>      g_test_add_data_func("/qga/guest-exec-invalid", &fix,
>                           test_qga_guest_exec_invalid);
>      g_test_add_data_func("/qga/guest-get-osinfo", &fix,
> --
> 2.39.1
>
>


-- 
Marc-André Lureau

Re: [PATCH 1/3] qga: test: Use absolute path to test data

[Daniel Xu]

Hi Marc-André,

Thanks for reviewing the series.

On Mon, Feb 27, 2023, at 1:16 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>> It looks like qga's working directory is in a tempdir. So the relative
>> path that the test case gives qga through the QGA_OS_RELEASE=
>> env variable does not resolve correctly.
>>
>> Fix by doing a poor man's path canonicalization of the test data file.
>>
>> Note we cannot use g_canonicalize_filename() b/c that helper was only
>> introduced in glib 2.58 and the current GLIB_VERSION_MAX_ALLOWED is
>> pinned to 2.56.
>>
>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>
> This breaks "meson test test-qga" for me. How do you run the tests?

Ah, thanks for the hint. I was running the qga tests in build/ with:

        $ ./tests/unit/test-qga

Using meson to drive the tests fixed it for me. I will drop this patch.

[...]

Thanks,
Daniel

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

[Daniel Xu]

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.

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

Re: [PATCH 3/3] qga: test: Add tests for `merge-output` flag

[Daniel Xu]

Hi,

On Mon, Feb 27, 2023, at 1:40 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 24, 2023 at 8:31 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>> This commit adds a test to ensure `merge-output` functions as expected.
>> We also add a negative test to ensure we haven't regressed previous
>> functionality.
>>
>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>
> Please check your patch with ASAN, you have use after-free introduced
> by this change:
> ==664972==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x621000135028 at pc 0x55e617a38b39 bp 0x7fff7fe85390 sp
> 0x7fff7fe85388
> READ of size 8 at 0x621000135028 thread T0
>     #0 0x55e617a38b38 in qdict_find ../qobject/qdict.c:96
>     #1 0x55e617a39bea in qdict_get ../qobject/qdict.c:164
>     #2 0x55e617a39bea in qdict_get_int ../qobject/qdict.c:209
>     #3 0x55e6179e2519 in test_qga_guest_exec ../tests/unit/test-qga.c:807
>     #4 0x7fbaa499dc7d in g_test_run_suite_internal
> (/lib64/libglib-2.0.so.0+0x84c7d)
>     #5 0x7fbaa499d9e4 in g_test_run_suite_internal
> (/lib64/libglib-2.0.so.0+0x849e4)
>     #6 0x7fbaa499e181 in g_test_run_suite (/lib64/libglib-2.0.so.0+0x85181)
>     #7 0x7fbaa49966ec in g_test_run (/lib64/libglib-2.0.so.0+0x7d6ec)
>     #8 0x55e6179da0ac in main ../tests/unit/test-qga.c:1083
>     #9 0x7fbaa384a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
>     #10 0x7fbaa384a5c8 in __libc_start_main@GLIBC_2.2.5
> (/lib64/libc.so.6+0x275c8)
>     #11 0x55e6179daf44 in _start
> (/home/elmarco/src/qemu/build/tests/unit/test-qga+0x1bbf44)

Ack. Will fix.

[...]

Thanks,
Daniel

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