[Spice-devel] [PATCH spice-server 01/12] Remove red_pipe_add_verb family function
Frediano Ziglio
fziglio at redhat.com
Wed Oct 19 16:05:13 UTC 2016
>
> 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 <fziglio at 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);
> > }
> >
>
More information about the Spice-devel
mailing list