[Spice-devel] [spice-server 1/2] worker: Use more local vars in dev_create_primary_surface

[Frediano Ziglio]

> 
> On Thu, May 24, 2018 at 09:35:16AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Thu, May 24, 2018 at 08:39:49AM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > There's already a 'display' variable equal to worker->display_channel
> > > > > which is not consistently used. This commit also adds a new 'channel'
> > > > > local variable to remove upcasts to RedChannel.
> > > > 
> > > > Not entirely true, channel is initialized doing the upcast, so there's
> > > > still the upcast.
> > > > I would move channel initialization inside the if, is not clear if
> > > > is related to the display channel or the cursor channel (note that
> > > > cursor_channel is used in this function).
> > > 
> > > Yes, was a bit worried by that, I can keep explicit RED_CHANNEL(display)
> > > rather than adding the 'channel' var as this makes it clearer.
> > > 
> > > Christophe
> > > 
> > 
> > Not much strong about it (the channel variable). Considering that
> > cursor_channel is used only on the last line is not that important.
> > The "inside if" comment is wrong taking into account patch 2/2.
> > The only strong comment is about the fact that this patch does
> > not remove the upcasts but more reduce them.
> 
> Ok, changed it to
> « This commit also adds a new 'channel' local variable to limit the
> number of upcasts to RedChannel. »
> 

Sounds good,

Acked-by: Frediano Ziglio <fziglio at redhat.com>

Frediano

> 
> > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > > > ---
> > > > >  server/red-worker.c | 8 ++++----
> > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > > index eb927f3e0..541110f83 100644
> > > > > --- a/server/red-worker.c
> > > > > +++ b/server/red-worker.c
> > > > > @@ -527,7 +527,8 @@ static void dev_create_primary_surface(RedWorker
> > > > > *worker,
> > > > > uint32_t surface_id,
> > > > >                                     line_0, surface.flags &
> > > > >                                     QXL_SURF_FLAG_KEEP_DATA, TRUE);
> > > > >      display_channel_set_monitors_config_to_primary(display);
> > > > >  
> > > > > -    CommonGraphicsChannel *common =
> > > > > COMMON_GRAPHICS_CHANNEL(worker->display_channel);
> > > > > +    CommonGraphicsChannel *common =
> > > > > COMMON_GRAPHICS_CHANNEL(display);
> > > > > +    RedChannel *channel = RED_CHANNEL(display);
> > > > >      if (display_is_connected(worker) &&
> > > > >          !common_graphics_channel_get_during_target_migrate(common))
> > > > >          {
> > > > >          /* guest created primary, so it will (hopefully) send a
> > > > >          monitors_config
> > > > > @@ -535,9 +536,8 @@ static void dev_create_primary_surface(RedWorker
> > > > > *worker,
> > > > > uint32_t surface_id,
> > > > >          if (!worker->driver_cap_monitors_config) {
> > > > >              display_channel_push_monitors_config(display);
> > > > >          }
> > > > > -
> > > > > red_channel_pipes_add_empty_msg(RED_CHANNEL(worker->display_channel),
> > > > > -                                        SPICE_MSG_DISPLAY_MARK);
> > > > > -        red_channel_push(RED_CHANNEL(worker->display_channel));
> > > > > +        red_channel_pipes_add_empty_msg(channel,
> > > > > SPICE_MSG_DISPLAY_MARK);
> > > > > +        red_channel_push(channel);
> > > > >      }
> > > > >  
> > > > >      cursor_channel_do_init(worker->cursor_channel);
> > > > 
>