| Message ID | 604ef5fd5bda8acdb837b5d28ec405e9fb0332a3.1695034158.git.dxu@dxuuu.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Add optional stream-output argument to guest-exec | expand |
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.
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
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
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 [...]
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
>
> [...]
>
>
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 >> >> [...] >>
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' }
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(-)