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