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@xxxxxxxxxx> > > > > > --- > > > > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel