Re: [Qemu-devel] [PATCH v4 17/20] console: make screendump asynchronous - Marc-André Lureau

From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	QEMU <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v4 17/20] console: make screendump asynchronous
Date: Mon, 15 Jul 2019 23:09:31 +0400	[thread overview]
Message-ID: <CAJ+F1CL8jD9-Pu_G60uRr77QnLR5KhTJ4PfbN__A9pyYBCUpTQ@mail.gmail.com> (raw)
In-Reply-To: <20190410084830.hiesbi6aumiwsdfk@sirius.home.kraxel.org>

On Wed, Apr 10, 2019 at 12:49 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > +static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
> > +{
> > +    Error *err = NULL;
> > +    DisplaySurface *surface;
> > +    Monitor *prev_mon = cur_mon;
>
> Why this is needed?
>

ppm_save() calls qemu_open() which may lookup fd associated to the
monitor: monitor_fdset_get_fd().

Interestingly, it seems fdset are not coupled with the current
monitor, so it's probably unnecessary to update the monitor to the one
associated with the command invocation.

> > +        /*
> > +         * FIXME: async save with coroutine? it would have to copy or
> > +         * lock the surface.
> > +         */
> > +        ppm_save(dump->filename, surface, &err);
>
> DisplaySurface is just a thin layer above pixman images these days.
> Pixman images are reference counted, so you can
> pixman_image_ref(surface->image) to make sure it doesn't disappear
> underneath you, then pass the pixman image to ppm_save.

ppm_save() is still synchronous. I suppose you suggested that for a
future async version. (note that in this case, ref the surface is
probably not sufficient, as it could be mutated while it is being
saved)

-- 
Marc-André Lureau