It seems that 

On Tue, 2016-10-18 at 10:09 +0100, Frediano Ziglio wrote:
> These functions were implementing the same stuff as empty
> messages functions provided by RedChannel so reuse them.
> 
> The implementation seems a bit different but the result
> is the same. Specifically:
> - RedEmptyMsgPipeItem::msg is int while RedVerbItem::verb was
>   uint16_t however this data goes into the message type which
>   is uint16_t (a 16 bit on the network protocol);

Should we change 'msg' to uint16_t then?

> - red_channel_client_send_empty_msg calls
>   red_channel_client_begin_send_message while red_marshall_verb
>   not. However cursor_channel_send_item and dcc_send_item call
>   red_channel_client_begin_send_message always after calling
>   red_marshall_verb and dcc_send_item calls.

This sentence ends kind of abrubtly and is a bit confusing.

>   You can be mistaken in dcc_send_item by
>   red_channel_client_send_message_pending check however this is
>   false only if during a marshalling a cache notify was added
>   which does not happen for red_marshall_verb;


Possible re-wording:

"Previously, there was one situation where dcc_send_item() did not call
red_channel_client_begin_send_message(): when
red_channel_client_send_message_pending() returned FALSE. This scenario
only occurs when a cache notify was added during marshalling"

However, I'm not totally sure what you mean by "cache notify was added"


> - when a PipeItem is created red_channel_client_pipe_add_empty_msg
>   calls red_channel_client_push while red_pipe_add_verb does not.
>   This actually make very few difference as this kind of item are

few -> little

"these kinds of items"

>   never removed from the queue and a push is forced in every case
>   running the event handler for the stream watch (see
>   prepare_pipe_add and red_channel_client_event).


I don't understand this part.



> 
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
>  server/common-graphics-channel.h | 34 +-----------------------------
> ----
>  server/cursor-channel.c          |  5 +----
>  server/dcc-send.c                |  3 ---
>  server/dcc.c                     |  2 +-
>  server/display-channel.c         |  2 +-
>  server/red-worker.c              |  4 ++--
>  6 files changed, 6 insertions(+), 44 deletions(-)
> 
> diff --git a/server/common-graphics-channel.h b/server/common-
> graphics-channel.h
> index 97cd63b..2a03414 100644
> --- a/server/common-graphics-channel.h
> +++ b/server/common-graphics-channel.h
> @@ -39,43 +39,11 @@ gboolean
> common_graphics_channel_get_during_target_migrate(CommonGraphicsChann
> el
>  QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel
> *self);
>  
>  enum {
> -    RED_PIPE_ITEM_TYPE_VERB = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
> -    RED_PIPE_ITEM_TYPE_INVAL_ONE,
> +    RED_PIPE_ITEM_TYPE_INVAL_ONE = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
>  
>      RED_PIPE_ITEM_TYPE_COMMON_LAST
>  };
>  
> -typedef struct RedVerbItem {
> -    RedPipeItem base;
> -    uint16_t verb;
> -} RedVerbItem;
> -
> -static inline void red_marshall_verb(RedChannelClient *rcc,
> RedVerbItem *item)
> -{
> -    red_channel_client_init_send_data(rcc, item->verb, NULL);
> -}
> -
> -static inline void red_pipe_add_verb(RedChannelClient* rcc, uint16_t
> verb)
> -{
> -    RedVerbItem *item = spice_new(RedVerbItem, 1);
> -
> -    red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_VERB);
> -    item->verb = verb;
> -    red_channel_client_pipe_add(rcc, &item->base);
> -}
> -
> -static inline void red_pipe_add_verb_proxy(RedChannelClient *rcc,
> gpointer data)
> -{
> -    uint16_t verb = GPOINTER_TO_UINT(data);
> -    red_pipe_add_verb(rcc, verb);
> -}
> -
> -
> -static inline void red_pipes_add_verb(RedChannel *channel, uint16_t
> verb)
> -{
> -    red_channel_apply_clients_data(channel, red_pipe_add_verb_proxy,
> GUINT_TO_POINTER(verb));
> -}
> -
>  CommonGraphicsChannel* common_graphics_channel_new(RedsState
> *server,
>                                                     QXLInstance *qxl,
>                                                     const
> SpiceCoreInterfaceInternal *core,
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 28a6d54..7df8763 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -292,9 +292,6 @@ static void
> cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_it
>      case RED_PIPE_ITEM_TYPE_INVAL_ONE:
>          red_marshall_inval(rcc, m, SPICE_CONTAINEROF(pipe_item,
> RedCacheItem, u.pipe_data));
>          break;
> -    case RED_PIPE_ITEM_TYPE_VERB:
> -        red_marshall_verb(rcc, SPICE_UPCAST(RedVerbItem,
> pipe_item));
> -        break;
>      case RED_PIPE_ITEM_TYPE_CURSOR_INIT:
>          cursor_channel_client_reset_cursor_cache(rcc);
>          red_marshall_cursor_init(ccc, m, pipe_item);
> @@ -392,7 +389,7 @@ void cursor_channel_reset(CursorChannel *cursor)
>      if (red_channel_is_connected(channel)) {
>          red_channel_pipes_add_type(channel,
> RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
>          if
> (!common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_C
> HANNEL(cursor))) {
> -            red_pipes_add_verb(channel, SPICE_MSG_CURSOR_RESET);
> +            red_channel_pipes_add_empty_msg(channel,
> SPICE_MSG_CURSOR_RESET);
>          }
>          if (!red_channel_wait_all_sent(&cursor->common.base,
>                                         COMMON_CLIENT_TIMEOUT)) {
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index e33f428..36521f0 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -2398,9 +2398,6 @@ void dcc_send_item(RedChannelClient *rcc,
> RedPipeItem *pipe_item)
>      case RED_PIPE_ITEM_TYPE_UPGRADE:
>          marshall_upgrade(rcc, m, SPICE_UPCAST(RedUpgradeItem,
> pipe_item));
>          break;
> -    case RED_PIPE_ITEM_TYPE_VERB:
> -        red_marshall_verb(rcc, SPICE_UPCAST(RedVerbItem,
> pipe_item));
> -        break;
>      case RED_PIPE_ITEM_TYPE_MIGRATE_DATA:
>          display_channel_marshall_migrate_data(rcc, m);
>          break;
> diff --git a/server/dcc.c b/server/dcc.c
> index 3519d2e..45ba81b 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -586,7 +586,7 @@ void dcc_start(DisplayChannelClient *dcc)
>          dcc_create_surface(dcc, 0);
>          dcc_push_surface_image(dcc, 0);
>          dcc_push_monitors_config(dcc);
> -        red_pipe_add_verb(rcc, SPICE_MSG_DISPLAY_MARK);
> +        red_channel_client_pipe_add_empty_msg(rcc,
> SPICE_MSG_DISPLAY_MARK);
>          dcc_create_all_streams(dcc);
>      }
>  
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 69edd35..3b8d66b 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1761,7 +1761,7 @@ void
> display_channel_destroy_surfaces(DisplayChannel *display)
>  
>      if (red_channel_is_connected(RED_CHANNEL(display))) {
>          red_channel_pipes_add_type(RED_CHANNEL(display),
> RED_PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE);
> -        red_pipes_add_verb(RED_CHANNEL(display),
> SPICE_MSG_DISPLAY_STREAM_DESTROY_ALL);
> +        red_channel_pipes_add_empty_msg(RED_CHANNEL(display),
> SPICE_MSG_DISPLAY_STREAM_DESTROY_ALL);
>      }
>  
>      display_channel_free_glz_drawables(display);
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 678856b..bbc971c 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -541,8 +541,8 @@ static void dev_create_primary_surface(RedWorker
> *worker, uint32_t surface_id,
>          if (!worker->driver_cap_monitors_config) {
>              red_worker_push_monitors_config(worker);
>          }
> -        red_pipes_add_verb(&worker->display_channel->common.base,
> -                           SPICE_MSG_DISPLAY_MARK);
> +        red_channel_pipes_add_empty_msg(&worker->display_channel-
> >common.base,
> +                                        SPICE_MSG_DISPLAY_MARK);
>          red_channel_push(&worker->display_channel->common.base);
>      }
>  
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to