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

[...]