[PATCH 0/3] qga: Add optional stream-output argument to guest-exec

[PATCH 0/3] qga: Add optional stream-output argument to guest-exec

[Daniel Xu]

Currently, commands run through guest-exec are "silent" until they
finish running. This is fine for short lived commands. But for commands
that take a while, this is a bad user experience.

Usually long running programs know that they will run for a while. To
improve user experience, they will typically print some kind of status
to output at a regular interval. So that the user knows that their
command isn't just hanging.

This patchset adds support for an optional stream-output parameter to
guest-exec. This causes subsequent calls to guest-exec-status to return
all buffered output. This allows downstream applications to be able to
relay "status" to the end user.

I also uncovered a latent memory leak bug with the added unit test. The
fix is in commit 1.


Daniel Xu (3):
  qga: Fix memory leak when output stream is unused
  qga: Add optional stream-output argument to guest-exec
  qga: test: Add test for guest-exec stream-output

 qga/commands.c        | 16 +++++++--
 qga/qapi-schema.json  |  7 +++-
 tests/unit/test-qga.c | 77 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH 1/3] qga: Fix memory leak when output stream is unused

[Daniel Xu]

If capture-output is requested but one of the channels goes unused (eg.
we attempt to capture stderr but the command never writes to stderr), we
can leak memory.

guest_exec_output_watch() is (from what I understand) unconditionally
called for both streams if output capture is requested. The first call
will always pass the `p->size == p->length` check b/c both values are
0. Then GUEST_EXEC_IO_SIZE bytes will be allocated for the stream.

But when we reap the exited process there's a `gei->err.length > 0`
check to actually free the buffer. Which does not get run if the command
doesn't write to the stream.

Fix by making free() unconditional.

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

diff --git a/qga/commands.c b/qga/commands.c
index 09c683e263..ce172edd2d 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -206,15 +206,15 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
 #endif
         if (gei->out.length > 0) {
             ges->out_data = g_base64_encode(gei->out.data, gei->out.length);
-            g_free(gei->out.data);
             ges->has_out_truncated = gei->out.truncated;
         }
+        g_free(gei->out.data);
 
         if (gei->err.length > 0) {
             ges->err_data = g_base64_encode(gei->err.data, gei->err.length);
-            g_free(gei->err.data);
             ges->has_err_truncated = gei->err.truncated;
         }
+        g_free(gei->err.data);
 
         QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
         g_free(gei);
-- 
2.41.0

[PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Daniel Xu]

Currently, commands run through guest-exec are "silent" until they
finish running. This is fine for short lived commands. But for commands
that take a while, this is a bad user experience.

Usually long running programs know that they will run for a while. To
improve user experience, they will typically print some kind of status
to output at a regular interval. So that the user knows that their
command isn't just hanging.

This commit adds support for an optional stream-output parameter to
guest-exec. This causes subsequent calls to guest-exec-status to return
all buffered output. This allows downstream applications to be able to
relay "status" to the end user.

If stream-output is requested, it is up to the guest-exec-status caller
to keep track of the last seen output position and slice the returned
output appropriately. This is fairly trivial for a client to do. And it
is a more reliable design than having QGA internally keep track of
position -- for the cases that the caller "loses" a response.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 qga/commands.c       | 12 ++++++++++++
 qga/qapi-schema.json |  7 ++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/qga/commands.c b/qga/commands.c
index ce172edd2d..8cabc2460f 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -97,6 +97,7 @@ struct GuestExecInfo {
     int64_t pid_numeric;
     gint status;
     bool has_output;
+    bool stream_output;
     bool finished;
     GuestExecIOData in;
     GuestExecIOData out;
@@ -218,6 +219,15 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
 
         QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
         g_free(gei);
+    } else if (gei->stream_output) {
+        if (gei->out.length > 0) {
+            ges->out_data = g_base64_encode(gei->out.data, gei->out.length);
+            ges->has_out_truncated = gei->out.truncated;
+        }
+        if (gei->err.length > 0) {
+            ges->err_data = g_base64_encode(gei->err.data, gei->err.length);
+            ges->has_err_truncated = gei->err.truncated;
+        }
     }
 
     return ges;
@@ -410,6 +420,7 @@ GuestExec *qmp_guest_exec(const char *path,
                        bool has_env, strList *env,
                        const char *input_data,
                        GuestExecCaptureOutput *capture_output,
+                       bool has_stream_output, bool stream_output,
                        Error **errp)
 {
     GPid pid;
@@ -485,6 +496,7 @@ GuestExec *qmp_guest_exec(const char *path,
 
     gei = guest_exec_info_add(pid);
     gei->has_output = has_output;
+    gei->stream_output = has_stream_output && stream_output;
     g_child_watch_add(pid, guest_exec_child_watch, gei);
 
     if (input_data) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b720dd4379..0a76e35082 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1315,13 +1315,18 @@
 # @capture-output: bool flag to enable capture of stdout/stderr of
 #     running process.  defaults to false.
 #
+# @stream-output: causes future guest-exec-status calls to always
+#     return current captured output rather than waiting to return
+#     it all when the command exits. defaults to false. (since: 8.2)
+#
 # Returns: PID on success.
 #
 # Since: 2.5
 ##
 { 'command': 'guest-exec',
   'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
-               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput' },
+               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput',
+               '*stream-output': 'bool' },
   'returns': 'GuestExec' }
 
 
-- 
2.41.0

[PATCH 3/3] qga: test: Add test for guest-exec stream-output

[Daniel Xu]

Add a test that simulates a long running process (by using a named pipe
to synchronize). This test ensures that full output is returned with
each call to guest-exec-status.

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

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 671e83cb86..455183e60b 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -952,6 +952,81 @@ static void test_qga_guest_exec_merged(gconstpointer fix)
 }
 #endif
 
+#if defined(G_OS_WIN32)
+static void test_qga_guest_exec_stream(gconstpointer fix)
+{
+}
+#else
+static void test_qga_guest_exec_stream(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    g_autoptr(QDict) ret = NULL;
+    g_autofree char *fifo_path = NULL;
+    g_autofree char *cmd = NULL;
+    QDict *val;
+    const gchar *out;
+    g_autofree guchar *decoded = NULL;
+    int64_t pid, exitcode;
+    bool exited;
+    gsize len;
+    int fifo;
+
+    fifo_path = g_strdup_printf("%s/fifo", fixture->test_dir);
+    g_assert_cmpint(mkfifo(fifo_path, 0644), ==, 0);
+
+    /* Echo two lines with a fifo barrier in between */
+    cmd = g_strdup_printf("echo line1; cat %s; echo line2;", fifo_path);
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/bash',"
+                 " 'arg': [ '-c', %s ],"
+                 " 'capture-output': 'stdout',"
+                 " 'stream-output': true } }",
+                 cmd);
+    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);
+
+    /* Give bash some time to run */
+    usleep(G_USEC_PER_SEC / 20);
+
+    /* Check first line comes out */
+    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");
+    g_assert_false(exited);
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 6);
+    g_assert_cmpstr((char *)decoded, ==, "line1\n");
+    g_free(decoded);
+    qobject_unref(ret);
+
+    /* Trigger second line */
+    fifo = open(fifo_path, O_WRONLY);
+    g_assert_cmpint(fifo, >=, 0);
+    close(fifo);
+    ret = wait_for_guest_exec_completion(fixture->fd, pid);
+
+    /* Check second line comes out after process exits */
+    val = qdict_get_qdict(ret, "return");
+    exited = qdict_get_bool(val, "exited");
+    g_assert_true(exited);
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 12);
+    g_assert_cmpstr((char *)decoded, ==, "line1\nline2\n");
+    g_assert_cmpint(unlink(fifo_path), ==, 0);
+}
+#endif
+
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -1127,6 +1202,8 @@ int main(int argc, char **argv)
                          test_qga_guest_exec_separated);
     g_test_add_data_func("/qga/guest-exec-merged", &fix,
                          test_qga_guest_exec_merged);
+    g_test_add_data_func("/qga/guest-exec-stream", &fix,
+                         test_qga_guest_exec_stream);
     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.41.0

Re: [PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Markus Armbruster]

Daniel Xu <dxu@dxuuu.xyz> writes:

> Currently, commands run through guest-exec are "silent" until they
> finish running. This is fine for short lived commands. But for commands
> that take a while, this is a bad user experience.
>
> Usually long running programs know that they will run for a while. To
> improve user experience, they will typically print some kind of status
> to output at a regular interval. So that the user knows that their
> command isn't just hanging.
>
> This commit adds support for an optional stream-output parameter to
> guest-exec. This causes subsequent calls to guest-exec-status to return
> all buffered output. This allows downstream applications to be able to
> relay "status" to the end user.
>
> If stream-output is requested, it is up to the guest-exec-status caller
> to keep track of the last seen output position and slice the returned
> output appropriately. This is fairly trivial for a client to do. And it
> is a more reliable design than having QGA internally keep track of
> position -- for the cases that the caller "loses" a response.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---

[...]

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b720dd4379..0a76e35082 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1315,13 +1315,18 @@
>  # @capture-output: bool flag to enable capture of stdout/stderr of
>  #     running process.  defaults to false.
>  #
> +# @stream-output: causes future guest-exec-status calls to always
> +#     return current captured output rather than waiting to return
> +#     it all when the command exits. defaults to false. (since: 8.2)

So guest-exec-status normally returns no captured output until the
process terminates, right?  Its documentation (shown below for
convenience) did not make me expect this!

> +#
>  # Returns: PID on success.
>  #
>  # Since: 2.5
>  ##
>  { 'command': 'guest-exec',
>    'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> -               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput' },
> +               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput',
> +               '*stream-output': 'bool' },
>    'returns': 'GuestExec' }

   ##
   # @GuestExecStatus:
   #
   # @exited: true if process has already terminated.
   #
   # @exitcode: process exit code if it was normally terminated.
   #
   # @signal: signal number (linux) or unhandled exception code (windows)
   #     if the process was abnormally terminated.
   #
   # @out-data: base64-encoded stdout of the process
   #
   # @err-data: base64-encoded stderr of the process Note: @out-data and
   #     @err-data are present only if 'capture-output' was specified for
   #     'guest-exec'
   #
   # @out-truncated: true if stdout was not fully captured due to size
   #     limitation.
   #
   # @err-truncated: true if stderr was not fully captured due to size
   #     limitation.
   #
   # Since: 2.5
   ##
   { 'struct': 'GuestExecStatus',
     'data': { 'exited': 'bool', '*exitcode': 'int', '*signal': 'int',
               '*out-data': 'str', '*err-data': 'str',
               '*out-truncated': 'bool', '*err-truncated': 'bool' }}
   ##
   # @guest-exec-status:
   #
   # Check status of process associated with PID retrieved via
   # guest-exec.  Reap the process and associated metadata if it has
   # exited.
   #
   # @pid: pid returned from guest-exec
   #
   # Returns: GuestExecStatus on success.
   #
   # Since: 2.5
   ##
   { 'command': 'guest-exec-status',
     'data':    { 'pid': 'int' },
     'returns': 'GuestExecStatus' }

Could you throw in a patch to clarify what exactly is returned while the
process is still running, and what only after it terminated?  It should
go first.

Re: [PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Daniel P. Berrangé]

On Mon, Sep 18, 2023 at 04:54:22AM -0600, Daniel Xu wrote:
> Currently, commands run through guest-exec are "silent" until they
> finish running. This is fine for short lived commands. But for commands
> that take a while, this is a bad user experience.
> 
> Usually long running programs know that they will run for a while. To
> improve user experience, they will typically print some kind of status
> to output at a regular interval. So that the user knows that their
> command isn't just hanging.
> 
> This commit adds support for an optional stream-output parameter to
> guest-exec. This causes subsequent calls to guest-exec-status to return
> all buffered output. This allows downstream applications to be able to
> relay "status" to the end user.
> 
> If stream-output is requested, it is up to the guest-exec-status caller
> to keep track of the last seen output position and slice the returned
> output appropriately. This is fairly trivial for a client to do. And it
> is a more reliable design than having QGA internally keep track of
> position -- for the cases that the caller "loses" a response.

I can understand why you want this incremental output facility,
but at the same time I wonder where we draw the line for QGA
with users needing a real shell session instead.

When there is a long lived command, then IMHO it is also likely
that there will be a need to kill the background running command
too.

We quickly end up re-inventing a shell in QGA if we go down this
route.


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 2/3] qga: Add optional stream-output argument to guest-exec

[Daniel Xu]

On Mon, Sep 18, 2023 at 05:05:03PM +0200, Markus Armbruster wrote:
> Daniel Xu <dxu@dxuuu.xyz> writes:
> 
> > Currently, commands run through guest-exec are "silent" until they
> > finish running. This is fine for short lived commands. But for commands
> > that take a while, this is a bad user experience.
> >
> > Usually long running programs know that they will run for a while. To
> > improve user experience, they will typically print some kind of status
> > to output at a regular interval. So that the user knows that their
> > command isn't just hanging.
> >
> > This commit adds support for an optional stream-output parameter to
> > guest-exec. This causes subsequent calls to guest-exec-status to return
> > all buffered output. This allows downstream applications to be able to
> > relay "status" to the end user.
> >
> > If stream-output is requested, it is up to the guest-exec-status caller
> > to keep track of the last seen output position and slice the returned
> > output appropriately. This is fairly trivial for a client to do. And it
> > is a more reliable design than having QGA internally keep track of
> > position -- for the cases that the caller "loses" a response.
> >
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> 
> [...]
> 
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index b720dd4379..0a76e35082 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1315,13 +1315,18 @@
> >  # @capture-output: bool flag to enable capture of stdout/stderr of
> >  #     running process.  defaults to false.
> >  #
> > +# @stream-output: causes future guest-exec-status calls to always
> > +#     return current captured output rather than waiting to return
> > +#     it all when the command exits. defaults to false. (since: 8.2)
> 
> So guest-exec-status normally returns no captured output until the
> process terminates, right?  Its documentation (shown below for
> convenience) did not make me expect this!

Right. You can see I made the same mistake [0] quite a few months ago
and realized it some time later.

[0]: https://github.com/danobi/vmtest/blob/73007280446cea3c823eb2dde78261501e6273ab/src/qemu.rs#L368-L406

> 
> > +#
> >  # Returns: PID on success.
> >  #
> >  # Since: 2.5
> >  ##
> >  { 'command': 'guest-exec',
> >    'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> > -               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput' },
> > +               '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput',
> > +               '*stream-output': 'bool' },
> >    'returns': 'GuestExec' }
> 
>    ##
>    # @GuestExecStatus:
>    #
>    # @exited: true if process has already terminated.
>    #
>    # @exitcode: process exit code if it was normally terminated.
>    #
>    # @signal: signal number (linux) or unhandled exception code (windows)
>    #     if the process was abnormally terminated.
>    #
>    # @out-data: base64-encoded stdout of the process
>    #
>    # @err-data: base64-encoded stderr of the process Note: @out-data and
>    #     @err-data are present only if 'capture-output' was specified for
>    #     'guest-exec'
>    #
>    # @out-truncated: true if stdout was not fully captured due to size
>    #     limitation.
>    #
>    # @err-truncated: true if stderr was not fully captured due to size
>    #     limitation.
>    #
>    # Since: 2.5
>    ##
>    { 'struct': 'GuestExecStatus',
>      'data': { 'exited': 'bool', '*exitcode': 'int', '*signal': 'int',
>                '*out-data': 'str', '*err-data': 'str',
>                '*out-truncated': 'bool', '*err-truncated': 'bool' }}
>    ##
>    # @guest-exec-status:
>    #
>    # Check status of process associated with PID retrieved via
>    # guest-exec.  Reap the process and associated metadata if it has
>    # exited.
>    #
>    # @pid: pid returned from guest-exec
>    #
>    # Returns: GuestExecStatus on success.
>    #
>    # Since: 2.5
>    ##
>    { 'command': 'guest-exec-status',
>      'data':    { 'pid': 'int' },
>      'returns': 'GuestExecStatus' }
> 
> Could you throw in a patch to clarify what exactly is returned while the
> process is still running, and what only after it terminated?  It should
> go first.
> 

Yes, will do.

Thanks,
Daniel

Re: [PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Daniel Xu]

Hi Daniel,

On Mon, Sep 18, 2023 at 04:14:47PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 18, 2023 at 04:54:22AM -0600, Daniel Xu wrote:
> > Currently, commands run through guest-exec are "silent" until they
> > finish running. This is fine for short lived commands. But for commands
> > that take a while, this is a bad user experience.
> > 
> > Usually long running programs know that they will run for a while. To
> > improve user experience, they will typically print some kind of status
> > to output at a regular interval. So that the user knows that their
> > command isn't just hanging.
> > 
> > This commit adds support for an optional stream-output parameter to
> > guest-exec. This causes subsequent calls to guest-exec-status to return
> > all buffered output. This allows downstream applications to be able to
> > relay "status" to the end user.
> > 
> > If stream-output is requested, it is up to the guest-exec-status caller
> > to keep track of the last seen output position and slice the returned
> > output appropriately. This is fairly trivial for a client to do. And it
> > is a more reliable design than having QGA internally keep track of
> > position -- for the cases that the caller "loses" a response.
> 
> I can understand why you want this incremental output facility,
> but at the same time I wonder where we draw the line for QGA
> with users needing a real shell session instead.

You mean interactive shell, right? If so, I would agree an interactive
shell is not a good fit for QGA.

But as it stands, a non-interactive shell works quite well (having
guest-exec run a bash script). I was the one who added the merged output
stream support a few months back. With merged output streams and this
streaming support, you can do some really neat things with QGA (see
below).

The primary reason I'm adding this support is for vmtest [0]. You can
find code for it here [1]. Basically what leveraging QGA does is allow
the vmtest implementation to reuse the same code for both kernel-only
(ie bzImage) and and image targets (eg qcow2). 

[0]: https://dxuuu.xyz/vmtest.html
[1]: https://github.com/danobi/vmtest

> 
> When there is a long lived command, then IMHO it is also likely
> that there will be a need to kill the background running command
> too.
> 
> We quickly end up re-inventing a shell in QGA if we go down this
> route.

I can understand if you don't want to bloat the QGA feature set, but
IMHO this change cleanly composes with the current implementation and
is easily unit testable (and comes with a test).

Per the discussion in the other thread, it could be argued that this
streaming feature is actually a bug fix -- the documentation seems to
imply otherwise, which both Markus and I have independently arrived
at. But I don't think we need to go into semantics like that :) .

But it does kinda imply from first principles that it is a reasonable
thing for guest-exec-status to provide. Perhaps it's too late to change
the existing behavior, so a flag is needed.

I hope my reasoning makes sense. And thanks for giving this a look.

Thanks,
Daniel

[...]

Re: [PATCH 1/3] qga: Fix memory leak when output stream is unused

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

 Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Mon, Sep 18, 2023 at 2:00 PM Daniel Xu <dxu@dxuuu.xyz> wrote:

> If capture-output is requested but one of the channels goes unused (eg.
> we attempt to capture stderr but the command never writes to stderr), we
> can leak memory.
>
> guest_exec_output_watch() is (from what I understand) unconditionally
> called for both streams if output capture is requested. The first call
> will always pass the `p->size == p->length` check b/c both values are
> 0. Then GUEST_EXEC_IO_SIZE bytes will be allocated for the stream.
>
> But when we reap the exited process there's a `gei->err.length > 0`
> check to actually free the buffer. Which does not get run if the command
> doesn't write to the stream.
>
> Fix by making free() unconditional.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  qga/commands.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 09c683e263..ce172edd2d 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -206,15 +206,15 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid,
> Error **errp)
>  #endif
>          if (gei->out.length > 0) {
>              ges->out_data = g_base64_encode(gei->out.data,
> gei->out.length);
> -            g_free(gei->out.data);
>              ges->has_out_truncated = gei->out.truncated;
>          }
> +        g_free(gei->out.data);
>
>          if (gei->err.length > 0) {
>              ges->err_data = g_base64_encode(gei->err.data,
> gei->err.length);
> -            g_free(gei->err.data);
>              ges->has_err_truncated = gei->err.truncated;
>          }
> +        g_free(gei->err.data);
>
>          QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
>          g_free(gei);
> --
> 2.41.0
>
>

[-- Attachment #2: Type: text/html, Size: 2497 bytes --]

Re: [PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

Hi Daniel,

As for me, the idea of using QGA as an interactive shell is not good.
I suggest using virtio-serial as a transport for stdin/stdout of your
process.
Examples:

https://stackoverflow.com/questions/68277557/qemu-virtio-virtconsole-devices-explained
    https://fedoraproject.org/wiki/Features/VirtioSerial

Is this solution good for your project?

Best Regards,
Konstantin Kostiuk.


On Mon, Sep 18, 2023 at 8:17 PM Daniel Xu <dxu@dxuuu.xyz> wrote:

> Hi Daniel,
>
> On Mon, Sep 18, 2023 at 04:14:47PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 18, 2023 at 04:54:22AM -0600, Daniel Xu wrote:
> > > Currently, commands run through guest-exec are "silent" until they
> > > finish running. This is fine for short lived commands. But for commands
> > > that take a while, this is a bad user experience.
> > >
> > > Usually long running programs know that they will run for a while. To
> > > improve user experience, they will typically print some kind of status
> > > to output at a regular interval. So that the user knows that their
> > > command isn't just hanging.
> > >
> > > This commit adds support for an optional stream-output parameter to
> > > guest-exec. This causes subsequent calls to guest-exec-status to return
> > > all buffered output. This allows downstream applications to be able to
> > > relay "status" to the end user.
> > >
> > > If stream-output is requested, it is up to the guest-exec-status caller
> > > to keep track of the last seen output position and slice the returned
> > > output appropriately. This is fairly trivial for a client to do. And it
> > > is a more reliable design than having QGA internally keep track of
> > > position -- for the cases that the caller "loses" a response.
> >
> > I can understand why you want this incremental output facility,
> > but at the same time I wonder where we draw the line for QGA
> > with users needing a real shell session instead.
>
> You mean interactive shell, right? If so, I would agree an interactive
> shell is not a good fit for QGA.
>
> But as it stands, a non-interactive shell works quite well (having
> guest-exec run a bash script). I was the one who added the merged output
> stream support a few months back. With merged output streams and this
> streaming support, you can do some really neat things with QGA (see
> below).
>
> The primary reason I'm adding this support is for vmtest [0]. You can
> find code for it here [1]. Basically what leveraging QGA does is allow
> the vmtest implementation to reuse the same code for both kernel-only
> (ie bzImage) and and image targets (eg qcow2).
>
> [0]: https://dxuuu.xyz/vmtest.html
> [1]: https://github.com/danobi/vmtest
>
> >
> > When there is a long lived command, then IMHO it is also likely
> > that there will be a need to kill the background running command
> > too.
> >
> > We quickly end up re-inventing a shell in QGA if we go down this
> > route.
>
> I can understand if you don't want to bloat the QGA feature set, but
> IMHO this change cleanly composes with the current implementation and
> is easily unit testable (and comes with a test).
>
> Per the discussion in the other thread, it could be argued that this
> streaming feature is actually a bug fix -- the documentation seems to
> imply otherwise, which both Markus and I have independently arrived
> at. But I don't think we need to go into semantics like that :) .
>
> But it does kinda imply from first principles that it is a reasonable
> thing for guest-exec-status to provide. Perhaps it's too late to change
> the existing behavior, so a flag is needed.
>
> I hope my reasoning makes sense. And thanks for giving this a look.
>
> Thanks,
> Daniel
>
> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 4927 bytes --]

Re: [PATCH 2/3] qga: Add optional stream-output argument to guest-exec

[Daniel Xu]

Hi Konstantin,

On Wed, Sep 27, 2023, at 2:43 AM, Konstantin Kostiuk wrote:
> Hi Daniel,
>
> As for me, the idea of using QGA as an interactive shell is not good.
> I suggest using virtio-serial as a transport for stdin/stdout of your 
> process. 
> Examples:
>     
> https://stackoverflow.com/questions/68277557/qemu-virtio-virtconsole-devices-explained
>     https://fedoraproject.org/wiki/Features/VirtioSerial
>
> Is this solution good for your project? 
>
> Best Regards,
> Konstantin Kostiuk.

Thanks for the tip. It looks like that'll work -- I'll look into it.

Daniel

>
>
> On Mon, Sep 18, 2023 at 8:17 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>> Hi Daniel,
>> 
>> On Mon, Sep 18, 2023 at 04:14:47PM +0100, Daniel P. Berrangé wrote:
>> > On Mon, Sep 18, 2023 at 04:54:22AM -0600, Daniel Xu wrote:
>> > > Currently, commands run through guest-exec are "silent" until they
>> > > finish running. This is fine for short lived commands. But for commands
>> > > that take a while, this is a bad user experience.
>> > > 
>> > > Usually long running programs know that they will run for a while. To
>> > > improve user experience, they will typically print some kind of status
>> > > to output at a regular interval. So that the user knows that their
>> > > command isn't just hanging.
>> > > 
>> > > This commit adds support for an optional stream-output parameter to
>> > > guest-exec. This causes subsequent calls to guest-exec-status to return
>> > > all buffered output. This allows downstream applications to be able to
>> > > relay "status" to the end user.
>> > > 
>> > > If stream-output is requested, it is up to the guest-exec-status caller
>> > > to keep track of the last seen output position and slice the returned
>> > > output appropriately. This is fairly trivial for a client to do. And it
>> > > is a more reliable design than having QGA internally keep track of
>> > > position -- for the cases that the caller "loses" a response.
>> > 
>> > I can understand why you want this incremental output facility,
>> > but at the same time I wonder where we draw the line for QGA
>> > with users needing a real shell session instead.
>> 
>> You mean interactive shell, right? If so, I would agree an interactive
>> shell is not a good fit for QGA.
>> 
>> But as it stands, a non-interactive shell works quite well (having
>> guest-exec run a bash script). I was the one who added the merged output
>> stream support a few months back. With merged output streams and this
>> streaming support, you can do some really neat things with QGA (see
>> below).
>> 
>> The primary reason I'm adding this support is for vmtest [0]. You can
>> find code for it here [1]. Basically what leveraging QGA does is allow
>> the vmtest implementation to reuse the same code for both kernel-only
>> (ie bzImage) and and image targets (eg qcow2). 
>> 
>> [0]: https://dxuuu.xyz/vmtest.html
>> [1]: https://github.com/danobi/vmtest
>> 
>> > 
>> > When there is a long lived command, then IMHO it is also likely
>> > that there will be a need to kill the background running command
>> > too.
>> > 
>> > We quickly end up re-inventing a shell in QGA if we go down this
>> > route.
>> 
>> I can understand if you don't want to bloat the QGA feature set, but
>> IMHO this change cleanly composes with the current implementation and
>> is easily unit testable (and comes with a test).
>> 
>> Per the discussion in the other thread, it could be argued that this
>> streaming feature is actually a bug fix -- the documentation seems to
>> imply otherwise, which both Markus and I have independently arrived
>> at. But I don't think we need to go into semantics like that :) .
>> 
>> But it does kinda imply from first principles that it is a reasonable
>> thing for guest-exec-status to provide. Perhaps it's too late to change
>> the existing behavior, so a flag is needed.
>> 
>> I hope my reasoning makes sense. And thanks for giving this a look.
>> 
>> Thanks,
>> Daniel
>> 
>> [...]
>>