[Spice-devel] [PATCH 3/7] worker: move surfaces to DisplayChannel

[Jonathon Jongsma]

On Thu, 2015-11-12 at 13:59 -0500, Frediano Ziglio wrote:
> > 
> > On Thu, 2015-11-12 at 11:16 -0500, Frediano Ziglio wrote:
> > > > 
> > > > On Thu, Nov 12, 2015 at 3:00 PM, Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > > 
> > > > > Ok. this one was painful.Note that in some cases, DCC_TO_DC should be
> > > > > made safer (there used to be a if !dcc guard in some places, although
> > > > > that looks wrong anyway)...
> > > > > ---
> > > > >  server/display-channel.c |   79 ++++
> > > > >  server/display-channel.h |   37 +-
> > > > >  server/red_worker.c      | 1116
> > > > >  ++++++++++++++++++++--------------------------
> > > > >  server/red_worker.h      |    2 +
> > > > >  4 files changed, 592 insertions(+), 642 deletions(-)
> > > > > 
> > > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > 
> > > ... omissis ...
> > > 
> > > > >  RedChannel* red_worker_get_cursor_channel(RedWorker *worker);
> > > > >  RedChannel* red_worker_get_display_channel(RedWorker *worker);
> > > > > +void red_worker_print_stats(RedWorker *worker);
> > > > > 
> > > > >  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> > > > >                                     const char *name,
> > > > > --
> > > > > 2.4.3
> > > > > 
> > > > > _______________________________________________
> > > > > Spice-devel mailing list
> > > > > Spice-devel at lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > 
> > > > I have ACKED this one in the other thread. Not sure if you are waiting
> > > > for one more ACK (I would wait for one more ACK).
> > > > Here is the link:
> > > > http://lists.freedesktop.org/archives/spice-devel/2015-November/023472.h
> > > > tml
> > > > 
> > > 
> > > Sorry, I forgot to update the patch. Can I proposed this?
> > > 
> > > 
> > > ---
> > >  server/red_worker.c | 20 ++++++++++++++++----
> > >  1 file changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 68ac527..f2b1446 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -877,7 +877,7 @@ static inline void current_remove(DisplayChannel
> > > *display,
> > > TreeItem *item)
> > >  {
> > >      TreeItem *now = item;
> > >  
> > > -    /* depth-first tree traversal, todo: do a to tree_foreach()? */
> > > +    /* depth-first tree traversal, TODO: do a to tree_foreach()? */
> > >      for (;;) {
> > >          Container *container = now->container;
> > >          RingItem *ring_item;
> > > @@ -3316,10 +3316,15 @@ static ImageItem
> > > *red_add_surface_area_image(DisplayChannelClient *dcc, int surf
> > >  
> > >  static void red_push_surface_image(DisplayChannelClient *dcc, int
> > >  surface_id)
> > >  {
> > > -    DisplayChannel *display = DCC_TO_DC(dcc);
> > > +    DisplayChannel *display;
> > >      SpiceRect area;
> > >      RedSurface *surface;
> > >  
> > > +    if (!dcc) {
> > > +        return;
> > > +    }
> > > +
> > > +    display = DCC_TO_DC(dcc);
> > 
> > Is there a valid reason for dcc to be NULL? If not, I'd suggest
> > g_return_if_fail() so that it prints a warning.
> > 
> > 
> > >      surface = &display->surfaces[surface_id];
> > >      if (!surface->context.canvas) {
> > >          return;
> > > @@ -7316,10 +7321,17 @@ static SurfaceCreateItem *get_surface_create_item(
> > >  
> > >  static inline void red_create_surface_item(DisplayChannelClient *dcc, int
> > > surface_id)
> > >  {
> > > -    DisplayChannel *display = dcc ? DCC_TO_DC(dcc) : NULL;
> > > +    DisplayChannel *display;
> > >      RedSurface *surface;
> > >      SurfaceCreateItem *create;
> > > -    uint32_t flags = is_primary_surface(DCC_TO_DC(dcc), surface_id) ?
> > > SPICE_SURFACE_FLAGS_PRIMARY : 0;
> > > +    uint32_t flags;
> > > +
> > > +    if (!dcc) {
> > > +        return;
> > > +    }
> > 
> > 
> > same comment here.
> > 
> > 
> > > +
> > > +    display = DCC_TO_DC(dcc);
> > > +    flags = is_primary_surface(DCC_TO_DC(dcc), surface_id) ?
> > > SPICE_SURFACE_FLAGS_PRIMARY : 0;
> > >  
> > >      /* don't send redundant create surface commands to client */
> > >      if (!dcc || display->common.during_target_migrate ||
> > 
> 
> Actually this second check is redundant.
> 
> The two checks for dcc NULL was already there before the patch this
> patch modify.
> The previous patch changed them in a wrong way. This was pointed out by
> Fabiano which proposed a patch and said "ok with these changes" (not literal
> citation). I proposed a different code that restore the checks.
> 
> Frediano


I'm kind of confused now. Perhaps I'll wait for a final patch to be proposed?