Re: [Spice-devel] [PATCH spice-server 01/12] Remove red_pipe_add_verb family function

[Frediano Ziglio]

> 
> 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?
> 

yes, could be a following up.

> > - 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.
> 

Basically red_channel_client_begin_send_message is called
in all cases. Could be

 - red_channel_client_send_empty_msg calls
   red_channel_client_begin_send_message while red_marshall_verb
   not. However red_marshall_verb calls are followed by either
   cursor_channel_send_item or dcc_send_item which always
   call red_channel_client_begin_send_message.

> >   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"
> 

By "cache notify" I mean the cache invalidation sent as urgent message.
Perhaps "cache notify" -> "cache invalidation".

If between reset_send_data and red_channel_client_send_message_pending
(all called by dcc_send_item) you discover that before sending the
message you have to free some cache data in the client (as the cache is
mostly full) begin_send_message is not called so neither is called
red_channel_client_begin_send_message for the item dcc_send_item is
sending however an empty message (or verb) does not send any image so
there can't be cache data to free.

Your rewording is mostly fine beside the previously. The behavior
is not changed, just it did not affect these empty messages.

> 
> > - 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.
> 

prepare_pipe_add schedule a call to red_channel_client_event when
there is space to write to the socket and red_channel_client_event
will "push" the data to the network socket. The only difference not
calling red_channel_client_push is that eventually the pipe item is
removed from the queue before red_channel_client_event is called but
only very few messages (mostly DrawablePipeItem) are removed from
the queue and not sent.

Frediano

> 
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  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, 

Re: [Spice-devel] [PATCH spice-server 01/12] Remove red_pipe_add_verb family function

[Jonathon Jongsma]

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 
> ---
>  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(>base, RED_PIPE_ITEM_TYPE_VERB);
> -item->verb = verb;
> -red_channel_client_pipe_add(rcc, >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,
> 

[Spice-devel] [PATCH spice-server 01/12] Remove red_pipe_add_verb family function

[Frediano Ziglio]

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);
- 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.
  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;
- 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
  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).

Signed-off-by: Frediano Ziglio 
---
 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(CommonGraphicsChannel
 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(>base, RED_PIPE_ITEM_TYPE_VERB);
-item->verb = verb;
-red_channel_client_pipe_add(rcc, >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_CHANNEL(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(>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,