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

[PATCH v3 0/4] 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.

Changes from v2:
* Error out if `merge-output` on windows guests
* Add FIXMEs for when glib is updated
* Fix memory leaks in qemu-keymap

Changes from v1:
* Drop invalid test fix
* Do not support `merge-output` on windows guests
* Fix a UAF in tests

Daniel Xu (4):
  crypto/luks: Initialize stack variable to silence warning
  qemu-keymap: Fix memory leaks
  qga: Add optional `merge-output` flag to guest-exec qapi
  qga: test: Add tests for `merge-output` flag

 crypto/block-luks.c   |   2 +-
 qemu-keymap.c         |   5 ++
 qga/commands.c        |  28 +++++++-
 qga/qapi-schema.json  |   6 +-
 tests/unit/test-qga.c | 157 +++++++++++++++++++++++++++++++++++++-----
 5 files changed, 177 insertions(+), 21 deletions(-)

-- 
2.39.1

[PATCH v3 1/4] crypto/luks: Initialize stack variable to silence warning

[Daniel Xu]

With `../configure --enable-sanitizers`, I was getting the following
build error:

        In file included from /usr/include/string.h:535,
                         from /home/dxu/dev/qemu/include/qemu/osdep.h:99,
                         from ../crypto/block-luks.c:21:
        In function ‘memset’,
            inlined from ‘qcrypto_block_luks_store_key’ at ../crypto/block-luks.c:843:9:
        /usr/include/bits/string_fortified.h:59:10: error: ‘splitkeylen’ may be used
        uninitialized [-Werror=maybe-uninitialized]
           59 |   return __builtin___memset_chk (__dest, __ch, __len,
              |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           60 |                                  __glibc_objsize0 (__dest));
              |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
        ../crypto/block-luks.c: In function ‘qcrypto_block_luks_store_key’:
        ../crypto/block-luks.c:699:12: note: ‘splitkeylen’ was declared here
          699 |     size_t splitkeylen;
              |            ^~~~~~~~~~~
        cc1: all warnings being treated as errors

The function is actually correct -- in the cleanup branch `splitkeylen`
usage is guarded by checking `splitkey` nullness. But the compiler is
not smart enough to realize that.

Fix warning by initializing the variable.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 crypto/block-luks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 5688783ab1..bfdef25c80 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -696,7 +696,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
     QCryptoBlockLUKS *luks = block->opaque;
     QCryptoBlockLUKSKeySlot *slot;
     g_autofree uint8_t *splitkey = NULL;
-    size_t splitkeylen;
+    size_t splitkeylen = 0;
     g_autofree uint8_t *slotkey = NULL;
     g_autoptr(QCryptoCipher) cipher = NULL;
     g_autoptr(QCryptoIVGen) ivgen = NULL;
-- 
2.39.1

[PATCH v3 2/4] qemu-keymap: Fix memory leaks

[Daniel Xu]

When building with `--enable-sanitizers`, I was getting quite a few
memory leak crashes from ASAN:

        [21/574] Generating pc-bios/keymaps/fr-ch with a custom command
        FAILED: pc-bios/keymaps/fr-ch
        /home/dxu/dev/qemu/build/qemu-keymap -f pc-bios/keymaps/fr-ch -l ch -v fr

        =================================================================
        ==3232549==ERROR: LeakSanitizer: detected memory leaks

        Direct leak of 1424 byte(s) in 1 object(s) allocated from:
            #0 0x7f32636bf411 in __interceptor_calloc /usr/src/debug/gcc/gcc/...
            #1 0x7f32635db73e  (/usr/lib/libxkbcommon.so.0+0x2273e)

Fix leaks by correctly decrementing refcounts on xkb structs.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 qemu-keymap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qemu-keymap.c b/qemu-keymap.c
index 229866e004..ed8cee3467 100644
--- a/qemu-keymap.c
+++ b/qemu-keymap.c
@@ -203,6 +203,7 @@ int main(int argc, char *argv[])
     map = xkb_keymap_new_from_names(ctx, &names, XKB_KEYMAP_COMPILE_NO_FLAGS);
     if (!map) {
         /* libxkbcommon prints error */
+        xkb_context_unref(ctx);
         exit(1);
     }
 
@@ -227,7 +228,11 @@ int main(int argc, char *argv[])
     state = xkb_state_new(map);
     xkb_keymap_key_for_each(map, walk_map, state);
     xkb_state_unref(state);
+    xkb_keymap_unref(map);
+    xkb_context_unref(ctx);
     state = NULL;
+    map = NULL;
+    ctx = NULL;
 
     /* add quirks */
     fprintf(outfile,
-- 
2.39.1

[PATCH v3 3/4] 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       | 28 ++++++++++++++++++++++++++--
 qga/qapi-schema.json |  6 +++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index 172826f8f8..cfce13d034 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(&sigact, 0, sizeof(struct sigaction));
     sigact.sa_handler = SIG_DFL;
 
@@ -384,6 +398,7 @@ GuestExec *qmp_guest_exec(const char *path,
                        bool has_env, strList *env,
                        const char *input_data,
                        bool has_capture_output, bool capture_output,
+                       bool has_merge_output, bool merge_output,
                        Error **errp)
 {
     GPid pid;
@@ -397,6 +412,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;
 
@@ -410,6 +426,14 @@ GuestExec *qmp_guest_exec(const char *path,
         }
     }
 
+#if defined(G_OS_WIN32)
+    /* FIXME: see comment in guest_exec_task_setup() */
+    if (has_merge) {
+        error_setg(errp, "merge-output unsupported on windows");
+        return NULL;
+    }
+#endif
+
     argv = guest_exec_get_args(&arglist, true);
     envp = has_env ? guest_exec_get_args(env, false) : NULL;
 
@@ -420,7 +444,7 @@ GuestExec *qmp_guest_exec(const char *path,
     }
 
     ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
-            guest_exec_task_setup, NULL, &pid, input_data ? &in_fd : NULL,
+            guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : NULL,
             has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
     if (!ret) {
         error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 796434ed34..9c2367acdf 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1211,6 +1211,9 @@
 # @input-data: data to be passed to process stdin (base64 encoded)
 # @capture-output: bool flag to enable capture of
 #                  stdout/stderr of running process. defaults to false.
+# @merge-output: bool flag to merge stdout/stderr of running process
+#                into stdout. only effective if used with @capture-output.
+#                not effective on windows guests. defaults to false. (since 8.0)
 #
 # Returns: PID on success.
 #
@@ -1218,7 +1221,8 @@
 ##
 { 'command': 'guest-exec',
   'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
-               '*input-data': 'str', '*capture-output': 'bool' },
+               '*input-data': 'str', '*capture-output': 'bool',
+               '*merge-output': 'bool' },
   'returns': 'GuestExec' }
 
 
-- 
2.39.1

[PATCH v3 4/4] 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 | 157 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 140 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index b4e0a14573..717f3d1103 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,23 +801,10 @@ 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 */
+    val = qdict_get_qdict(ret, "return");
     exitcode = qdict_get_int(val, "exitcode");
     g_assert_cmpint(exitcode, ==, 0);
     out = qdict_get_str(val, "out-data");
@@ -802,6 +813,114 @@ 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);
+
+    val = qdict_get_qdict(ret, "return");
+    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");
+}
+
+#if defined(G_OS_WIN32)
+static void test_qga_guest_exec_output_merge(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
+    const gchar *class, *desc;
+    g_autofree guchar *decoded = NULL;
+
+    /* 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);
+    val = qdict_get_qdict(ret, "error");
+    g_assert_nonnull(val);
+    class = qdict_get_str(val, "class");
+    desc = qdict_get_str(val, "desc");
+    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpint(strlen(desc), >, 0);
+}
+#else
+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);
+
+    val = qdict_get_qdict(ret, "return");
+    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);
+}
+#endif
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -972,6 +1091,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 v3 2/4] qemu-keymap: Fix memory leaks

[Marc-André Lureau]

On Wed, Mar 1, 2023 at 12:48 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> When building with `--enable-sanitizers`, I was getting quite a few
> memory leak crashes from ASAN:
>
>         [21/574] Generating pc-bios/keymaps/fr-ch with a custom command
>         FAILED: pc-bios/keymaps/fr-ch
>         /home/dxu/dev/qemu/build/qemu-keymap -f pc-bios/keymaps/fr-ch -l ch -v fr
>
>         =================================================================
>         ==3232549==ERROR: LeakSanitizer: detected memory leaks
>
>         Direct leak of 1424 byte(s) in 1 object(s) allocated from:
>             #0 0x7f32636bf411 in __interceptor_calloc /usr/src/debug/gcc/gcc/...
>             #1 0x7f32635db73e  (/usr/lib/libxkbcommon.so.0+0x2273e)
>
> Fix leaks by correctly decrementing refcounts on xkb structs.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qemu-keymap.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/qemu-keymap.c b/qemu-keymap.c
> index 229866e004..ed8cee3467 100644
> --- a/qemu-keymap.c
> +++ b/qemu-keymap.c
> @@ -203,6 +203,7 @@ int main(int argc, char *argv[])
>      map = xkb_keymap_new_from_names(ctx, &names, XKB_KEYMAP_COMPILE_NO_FLAGS);
>      if (!map) {
>          /* libxkbcommon prints error */
> +        xkb_context_unref(ctx);
>          exit(1);
>      }
>
> @@ -227,7 +228,11 @@ int main(int argc, char *argv[])
>      state = xkb_state_new(map);
>      xkb_keymap_key_for_each(map, walk_map, state);
>      xkb_state_unref(state);
> +    xkb_keymap_unref(map);
> +    xkb_context_unref(ctx);
>      state = NULL;
> +    map = NULL;
> +    ctx = NULL;
>
>      /* add quirks */
>      fprintf(outfile,
> --
> 2.39.1
>


-- 
Marc-André Lureau

Re: [PATCH v3 1/4] crypto/luks: Initialize stack variable to silence warning

[Daniel P. Berrangé]

On Tue, Feb 28, 2023 at 01:48:01PM -0700, Daniel Xu wrote:
> With `../configure --enable-sanitizers`, I was getting the following
> build error:
> 
>         In file included from /usr/include/string.h:535,
>                          from /home/dxu/dev/qemu/include/qemu/osdep.h:99,
>                          from ../crypto/block-luks.c:21:
>         In function ‘memset’,
>             inlined from ‘qcrypto_block_luks_store_key’ at ../crypto/block-luks.c:843:9:
>         /usr/include/bits/string_fortified.h:59:10: error: ‘splitkeylen’ may be used
>         uninitialized [-Werror=maybe-uninitialized]
>            59 |   return __builtin___memset_chk (__dest, __ch, __len,
>               |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>            60 |                                  __glibc_objsize0 (__dest));
>               |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>         ../crypto/block-luks.c: In function ‘qcrypto_block_luks_store_key’:
>         ../crypto/block-luks.c:699:12: note: ‘splitkeylen’ was declared here
>           699 |     size_t splitkeylen;
>               |            ^~~~~~~~~~~
>         cc1: all warnings being treated as errors
> 
> The function is actually correct -- in the cleanup branch `splitkeylen`
> usage is guarded by checking `splitkey` nullness. But the compiler is
> not smart enough to realize that.
> 
> Fix warning by initializing the variable.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  crypto/block-luks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v3 3/4] qga: Add optional `merge-output` flag to guest-exec qapi

[Daniel P. Berrangé]

On Tue, Feb 28, 2023 at 01:48:03PM -0700, 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 <dxu@dxuuu.xyz>
> ---
>  qga/commands.c       | 28 ++++++++++++++++++++++++++--
>  qga/qapi-schema.json |  6 +++++-
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index 172826f8f8..cfce13d034 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(&sigact, 0, sizeof(struct sigaction));
>      sigact.sa_handler = SIG_DFL;
>  
> @@ -384,6 +398,7 @@ GuestExec *qmp_guest_exec(const char *path,
>                         bool has_env, strList *env,
>                         const char *input_data,
>                         bool has_capture_output, bool capture_output,
> +                       bool has_merge_output, bool merge_output,
>                         Error **errp)
>  {
>      GPid pid;
> @@ -397,6 +412,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;
>  
> @@ -410,6 +426,14 @@ GuestExec *qmp_guest_exec(const char *path,
>          }
>      }
>  
> +#if defined(G_OS_WIN32)
> +    /* FIXME: see comment in guest_exec_task_setup() */
> +    if (has_merge) {
> +        error_setg(errp, "merge-output unsupported on windows");
> +        return NULL;
> +    }
> +#endif
> +
>      argv = guest_exec_get_args(&arglist, true);
>      envp = has_env ? guest_exec_get_args(env, false) : NULL;
>  
> @@ -420,7 +444,7 @@ GuestExec *qmp_guest_exec(const char *path,
>      }
>  
>      ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> -            guest_exec_task_setup, NULL, &pid, input_data ? &in_fd : NULL,
> +            guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : NULL,
>              has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
>      if (!ret) {
>          error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 796434ed34..9c2367acdf 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1211,6 +1211,9 @@
>  # @input-data: data to be passed to process stdin (base64 encoded)
>  # @capture-output: bool flag to enable capture of
>  #                  stdout/stderr of running process. defaults to false.
> +# @merge-output: bool flag to merge stdout/stderr of running process
> +#                into stdout. only effective if used with @capture-output.
> +#                not effective on windows guests. defaults to false. (since 8.0)
>  #
>  # Returns: PID on success.
>  #
> @@ -1218,7 +1221,8 @@
>  ##
>  { 'command': 'guest-exec',
>    'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> -               '*input-data': 'str', '*capture-output': 'bool' },
> +               '*input-data': 'str', '*capture-output': 'bool',
> +               '*merge-output': 'bool' },
>    'returns': 'GuestExec' }

I feel like 'merge-output' is a somewhat specialized policy. What if
we want to capture only stderr, and discard stdout, or vica-verca ?
IMHO, the original 'capture-output' field was poorly designed and
should have been an enum. I believe we can retrofit greater
flexibility by using an enum plus and alternate thus:

 { 'enum': 'GuestExecCaptureOutputMode',
   'data': [ 'none', 'stdout', 'stderr', 'all' ] }

 { 'alternate': 'GuestExecCaptureOutput',
   'data': { 'flag': 'bool',
             'mode': 'GuestExecCaptureOutputMode'} }

And then change 'guest-exec':

    '*capture-output': 'GuestExecCaptureOutput'

the use of the alternate makes this backwards compatible, as we can
distinguish a JSON bool on the wire from an enum represented as a
string.

This should be easy to implement, as it just involves selectively
toggling G_SPAWN_STDOUT_TO_DEV_NULL / G_SPAWN_STDERR_TO_DEV_NULL
flags, instead of setting them both together.

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 v3 4/4] qga: test: Add tests for `merge-output` flag

[Daniel P. Berrangé]

On Tue, Feb 28, 2023 at 01:48:04PM -0700, Daniel Xu 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>
> ---
>  tests/unit/test-qga.c | 157 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 140 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index b4e0a14573..717f3d1103 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,23 +801,10 @@ 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 */
> +    val = qdict_get_qdict(ret, "return");
>      exitcode = qdict_get_int(val, "exitcode");
>      g_assert_cmpint(exitcode, ==, 0);
>      out = qdict_get_str(val, "out-data");
> @@ -802,6 +813,114 @@ 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 } }");

Surely /bin/bash isn't going to exist on most (all?) Windows deployments,
and so this test will fail on Windows builds ?


> +    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);
> +
> +    val = qdict_get_qdict(ret, "return");
> +    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");
> +}
> +
> +#if defined(G_OS_WIN32)
> +static void test_qga_guest_exec_output_merge(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    g_autoptr(QDict) ret = NULL;
> +    QDict *val;
> +    const gchar *class, *desc;
> +    g_autofree guchar *decoded = NULL;
> +
> +    /* 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;' ],"

Looks odd to be invoking bash, though in this case its harmless as we
don't get that far. Still would suggest using 'cmd.exe' as the
example though.

> +                 " 'capture-output': true,"
> +                 " 'merge-output': true } }");
> +
> +    g_assert_nonnull(ret);
> +    val = qdict_get_qdict(ret, "error");
> +    g_assert_nonnull(val);
> +    class = qdict_get_str(val, "class");
> +    desc = qdict_get_str(val, "desc");
> +    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpint(strlen(desc), >, 0);
> +}
> +#else
> +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);
> +
> +    val = qdict_get_qdict(ret, "return");
> +    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);
> +}
> +#endif
> +
>  static void test_qga_guest_exec_invalid(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> @@ -972,6 +1091,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
> 
> 

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 v3 3/4] qga: Add optional `merge-output` flag to guest-exec qapi

[Daniel Xu]

Hi Daniel,

On Wed, Mar 01, 2023 at 09:03:53AM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 28, 2023 at 01:48:03PM -0700, 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 <dxu@dxuuu.xyz>
> > ---
> >  qga/commands.c       | 28 ++++++++++++++++++++++++++--
> >  qga/qapi-schema.json |  6 +++++-
> >  2 files changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qga/commands.c b/qga/commands.c
> > index 172826f8f8..cfce13d034 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(&sigact, 0, sizeof(struct sigaction));
> >      sigact.sa_handler = SIG_DFL;
> >  
> > @@ -384,6 +398,7 @@ GuestExec *qmp_guest_exec(const char *path,
> >                         bool has_env, strList *env,
> >                         const char *input_data,
> >                         bool has_capture_output, bool capture_output,
> > +                       bool has_merge_output, bool merge_output,
> >                         Error **errp)
> >  {
> >      GPid pid;
> > @@ -397,6 +412,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;
> >  
> > @@ -410,6 +426,14 @@ GuestExec *qmp_guest_exec(const char *path,
> >          }
> >      }
> >  
> > +#if defined(G_OS_WIN32)
> > +    /* FIXME: see comment in guest_exec_task_setup() */
> > +    if (has_merge) {
> > +        error_setg(errp, "merge-output unsupported on windows");
> > +        return NULL;
> > +    }
> > +#endif
> > +
> >      argv = guest_exec_get_args(&arglist, true);
> >      envp = has_env ? guest_exec_get_args(env, false) : NULL;
> >  
> > @@ -420,7 +444,7 @@ GuestExec *qmp_guest_exec(const char *path,
> >      }
> >  
> >      ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> > -            guest_exec_task_setup, NULL, &pid, input_data ? &in_fd : NULL,
> > +            guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : NULL,
> >              has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
> >      if (!ret) {
> >          error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 796434ed34..9c2367acdf 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1211,6 +1211,9 @@
> >  # @input-data: data to be passed to process stdin (base64 encoded)
> >  # @capture-output: bool flag to enable capture of
> >  #                  stdout/stderr of running process. defaults to false.
> > +# @merge-output: bool flag to merge stdout/stderr of running process
> > +#                into stdout. only effective if used with @capture-output.
> > +#                not effective on windows guests. defaults to false. (since 8.0)
> >  #
> >  # Returns: PID on success.
> >  #
> > @@ -1218,7 +1221,8 @@
> >  ##
> >  { 'command': 'guest-exec',
> >    'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> > -               '*input-data': 'str', '*capture-output': 'bool' },
> > +               '*input-data': 'str', '*capture-output': 'bool',
> > +               '*merge-output': 'bool' },
> >    'returns': 'GuestExec' }
> 
> I feel like 'merge-output' is a somewhat specialized policy. What if
> we want to capture only stderr, and discard stdout, or vica-verca ?
> IMHO, the original 'capture-output' field was poorly designed and
> should have been an enum. I believe we can retrofit greater
> flexibility by using an enum plus and alternate thus:
> 
>  { 'enum': 'GuestExecCaptureOutputMode',
>    'data': [ 'none', 'stdout', 'stderr', 'all' ] }
> 
>  { 'alternate': 'GuestExecCaptureOutput',
>    'data': { 'flag': 'bool',
>              'mode': 'GuestExecCaptureOutputMode'} }
> 
> And then change 'guest-exec':
> 
>     '*capture-output': 'GuestExecCaptureOutput'
> 
> the use of the alternate makes this backwards compatible, as we can
> distinguish a JSON bool on the wire from an enum represented as a
> string.
> 
> This should be easy to implement, as it just involves selectively
> toggling G_SPAWN_STDOUT_TO_DEV_NULL / G_SPAWN_STDERR_TO_DEV_NULL
> flags, instead of setting them both together.

Thank you for taking a look. What you're describing makes sense to me.

I'll split out the first 2 sanitizer fixes in the series today. I'll
rework the rest of the patches per you suggestion likely this weekend.

Thanks,
Daniel

[...]