Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous

[Marc-André Lureau]

Hi

On Thu, Apr 12, 2018 at 4:49 PM Dr. David Alan Gilbert
<address@hidden> wrote:
>
> * Marc-André Lureau (address@hidden) wrote:
> > Make screendump asynchronous to provide correct screendumps.
> >
> > HMP doesn't have async support, so it has to remain synchronous and
> > potentially incorrect to avoid potential races.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
>
> Hi,
>   Looking at this and the previous pair of patches, I think we'd be
> better if we defined that async commands took a callback function
> pointer and data that they call.
>
>   Here we've got 'graphic_hw_update_done' that explicitly walks qmp
> lists; but I think it's better just to pass graphic_hw_update() the
> completion data together with a callback function and it calls that when
> it's done.

The calling context is in QmpReturn: it will handle further dispatch
on the associated session (it has the associated cb+data, if you
will).

gfx_update_async() doesn't have cb+data to associate the call with a
finish handler. Further down, spice doesn't keep the request "context"
either. All it does is call graphic_hw_update_done() when update is
finished, that's why we walk the list of pending screendumps here.

We could move the pending list of async screendumps down to spice (and
other console renderers), but that would make the code more
complicated & duplicated for no strong reason.

>
> (Also isn't it a little bit of a race, calling graphic_hw_update() and
> *then* adding the entry to the list? Can't it end up calling the _done()
> before we've added it to the list).

Yes, in theory it could. In practice that won't happen today, but we
may change things later and not realize we "broke" screendump. So I
will rearrange the code to add it to the pending list before calling
graphic_hw_update().


>
> Also, do we need a list of outstanding screendumps, or should we just
> have a list of async commands.

We have both, mostly for simplicity and flexibility reasons. If we
removed the outstanding screendump list, we would have to query it
from the session.

Thanks for the review, I'll send an update revision.


--
Marc-André Lureau