[Spice-devel] [PATCH 3.12/12] Various minor style changes to worker and cursor channel

[Jonathon Jongsma]

On Fri, 2015-10-30 at 03:39 -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > ---
> >  server/cursor-channel.c | 30 +++++++++++++++++-------------
> >  server/red_worker.c     |  7 ++++---
> >  2 files changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index ecb49b2..9935c00 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -232,17 +232,17 @@ static void
> > red_marshall_cursor_init(RedChannelClient
> > *rcc, SpiceMarshaller *bas
> >  }
> >  
> >  static void cursor_marshall(RedChannelClient *rcc,
> > -                   SpiceMarshaller *m, CursorPipeItem
> > *cursor_pipe_item)
> > +                            SpiceMarshaller *m, CursorPipeItem
> > *cursor_pipe_item)
> >  {
> >      CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc
> > ->channel,
> >      CursorChannel, common.base);
> >      CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> > -    CursorItem *cursor = cursor_pipe_item->cursor_item;
> > +    CursorItem *item = cursor_pipe_item->cursor_item;
> >      PipeItem *pipe_item = &cursor_pipe_item->base;
> >      RedCursorCmd *cmd;
> >  
> >      spice_return_if_fail(cursor_channel);
> >  
> > -    cmd = cursor->red_cursor;
> > +    cmd = item->red_cursor;
> >      switch (cmd->type) {
> >      case QXL_CURSOR_MOVE:
> >          {
> > @@ -261,7 +261,7 @@ static void cursor_marshall(RedChannelClient
> > *rcc,
> >              cursor_set.position = cmd->u.set.position;
> >              cursor_set.visible = cursor_channel->cursor_visible;
> >  
> > -            cursor_fill(ccc, &cursor_set.cursor, cursor, &info);
> > +            cursor_fill(ccc, &cursor_set.cursor, item, &info);
> >              spice_marshall_msg_cursor_set(m, &cursor_set);
> >              add_buf_from_info(m, &info);
> >              break;
> > @@ -285,7 +285,7 @@ static void cursor_marshall(RedChannelClient
> > *rcc,
> >  }
> >  
> >  static inline void red_marshall_inval(RedChannelClient *rcc,
> > -        SpiceMarshaller *base_marshaller, CacheItem *cach_item)
> > +                                      SpiceMarshaller
> > *base_marshaller,
> > CacheItem *cach_item)
> >  {
> >      SpiceMsgDisplayInvalOne inval_one;
> >  
> > @@ -446,10 +446,11 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >          return;
> >      }
> >  
> > -    if (red_channel_is_connected(&cursor->common.base) &&
> > (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> > -                                   cursor_cmd->type !=
> > QXL_CURSOR_MOVE ||
> > cursor_show)) {
> > -        red_channel_pipes_new_add(&cursor->common.base,
> > new_cursor_pipe_item,
> > -                                  (void*)cursor_item);
> > +    if (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
> > +        || cursor_cmd->type != QXL_CURSOR_MOVE
> > +        || cursor_show) {
> > +        red_channel_pipes_new_add(&cursor->common.base,
> > +                                  new_cursor_pipe_item,
> > cursor_item);
> >      }
> 
> Why silently remove the red_channel_is_connected check?

Yes, I had the same question and was going to comment on it as well.
Marc-Andre, if you're reading this, do you remember?


> 
> >  
> >      cursor_item_unref(cursor_item);
> > @@ -457,21 +458,24 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >  
> >  void cursor_channel_reset(CursorChannel *cursor)
> >  {
> > +    RedChannel *channel = (RedChannel *)cursor;
> > +
> 
> I would prefer channel = &cursor->common.base.
> 
> > +    spice_return_if_fail(cursor);
> > +
> >      cursor_set_item(cursor, NULL);
> >      cursor->cursor_visible = TRUE;
> >      cursor->cursor_position.x = cursor->cursor_position.y = 0;
> >      cursor->cursor_trail_length = cursor->cursor_trail_frequency =
> > 0;
> >  
> > -    if (red_channel_is_connected(&cursor->common.base)) {
> > -        red_channel_pipes_add_type(&cursor->common.base,
> > -                                  
> >  PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> > +    if (red_channel_is_connected(channel)) {
> > +        red_channel_pipes_add_type(&cursor->common.base,
> 
> change both to use channel or none here.
> 
> > PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> >          if (!cursor->common.during_target_migrate) {
> >              red_pipes_add_verb(&cursor->common.base,
> >              SPICE_MSG_CURSOR_RESET);
> >          }
> >          if (!red_channel_wait_all_sent(&cursor->common.base,
> 
> another occurrence.
> 
> >                                         DISPLAY_CLIENT_TIMEOUT)) {
> >              red_channel_apply_clients(&cursor->common.base,
> > -
> > red_channel_client_disconnect_if_pending_send);
> > +
> > red_channel_client_disconnect_if_pending_send);
> >          }
> >      }
> >  }
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index b93676e..8bb048b 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -10272,10 +10272,9 @@ static void
> > dev_create_primary_surface(RedWorker
> > *worker, uint32_t surface_id,
> >          red_channel_push(&worker->display_channel->common.base);
> >      }
> >  
> > -    if (cursor_is_connected(worker) &&
> > !worker->cursor_channel->common.during_target_migrate) {
> > +    if (!worker->cursor_channel->common.during_target_migrate)
> 
> check removed without explanation. Also I think mostly if use blocks
> so
> I don't agree with this style change.
> 
> >          red_channel_pipes_add_type(&worker->cursor_channel
> > ->common.base,
> >                                     PIPE_ITEM_TYPE_CURSOR_INIT);
> > -    }
> >  }
> >  
> >  void handle_dev_create_primary_surface(void *opaque, void
> > *payload)
> > @@ -10474,7 +10473,9 @@ void handle_dev_oom(void *opaque, void
> > *payload)
> >  
> >  void handle_dev_reset_cursor(void *opaque, void *payload)
> >  {
> > -    cursor_channel_reset(((RedWorker*)opaque)->cursor_channel);
> > +    RedWorker *worker = opaque;
> > +
> > +    cursor_channel_reset(worker->cursor_channel);
> >  }
> >  
> >  void handle_dev_reset_image_cache(void *opaque, void *payload)
> > --
> > 2.4.3
> 
> Frediano