[Spice-devel] [PATCH spice-server 1/2] red-qxl: Remove AsyncCommand

[Christophe Fergeau]

On Mon, Sep 18, 2017 at 11:17:16AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 18, 2017 at 06:19:03AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Mon, Sep 11, 2017 at 09:08:54AM +0100, Frediano Ziglio wrote:
> > > > > This structure was used to store the cookie for the async
> > > > > reply and the message for the generic async callback.
> > > > 
> > > > > Most async messages do not require extra action beside
> > > > > sending back the cookie for the reply so instead of using
> > > > > a switch inline the replies and remove store the cookie
> > > > > directory instead of a pointer to AsyncCommand.
> > > > 
> > > > "instead of having a switch on the message type in
> > > > red_qxl_async_complete, this commit moves the message-specific behaviour
> > > > to the callers, which allows us to store the cookie directly in
> > > > RedWorkerMessageAsync
> > > > rather than needing an intermediate AsyncCommand structure"
> > > > 
> > > > (though I guess we could have exactly the same explanation for moving
> > > > both the cookie and the message type to RedWorkerMessageAsync).
> > > > 
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > >  server/red-qxl.c    | 71
> > > > >  +++++++++++++----------------------------------------
> > > > >  server/red-qxl.h    |  1 +
> > > > >  server/red-worker.c | 18 ++++++++------
> > > > >  server/red-worker.h |  5 ++--
> > > > >  4 files changed, 31 insertions(+), 64 deletions(-)
> > > > > 
> > > > > Not a regression but this change shows that
> > > > > red_qxl_destroy_primary_surface_complete and
> > > > > red_qxl_create_primary_surface_complete are called from the worker
> > > > > thread. This possibly can be a race condition as they call some
> > > > > functions in red-qxl.c and reds.c that possibly use structures that
> > > > > should only be used in the main thread.
> > > > 
> > > > I would add a comment to the functions that you made public to mention
> > > > that they are being called from the worker thread.
> > > > 
> > > 
> > > Not exactly "public", they are declared in red-worker.h which is
> > > supposed to be used for RedQxl <-> RedWorker.
> > > But this is clear if you look for declaration, not if you just read
> > > the red-qxl.c source.
> > > What about a
> > > 
> > > /* used by RedWorker */
> > > 
> > > in front of red_qxl_destroy_primary_surface_complete and
> > > red_qxl_create_primary_surface_complete ?
> > 
> > Dunno, to me the only useful information is which thread the function
> > should be called from, which is why I was suggesting the annotation.
> > 
> > Christophe
> > 
> 
> Well, I think these function should NOT be called from the working
> thread (as said this is not a regression).

Depends from how you look at it :) The
red_qxl_destroy_primary_surface_complete() API entry point *should* be
called from the worker thread. But the current
red_qxl_destroy_primary_surface_complete() implementation *should not*
be called from the worker thread :)

Christophe