[Spice-devel] [PATCH spice-server v2] Remove red_pipe_add_verb family function

[Frediano Ziglio]

> 
> Hey,
> 
> On Wed, Oct 26, 2016 at 09:45:15AM +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);
> 
> > - 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
> 
> I think you are missing a "does" -> "while red_marshall_verb does not"
> 
> >   cursor_channel_send_item or dcc_send_item which always
> >   call red_channel_client_begin_send_message.
> 
> "always sent" I think.
> 
> >   Note that in dcc_send_item when an empty message is sent
> >   red_channel_client_send_message_pending always returns
> >   true;
> 
> I think this is not exactly true, the red_marshall_verb calls are done
> from cursor_channel_send_item or dcc_send_item, which end with a call to
> red_channel_client_begin_send_message()
> 

dcc_send_item finish with

    // a message is pending
    if (red_channel_client_send_message_pending(rcc)) {
        begin_send_message(rcc);
    }

in case of verb red_channel_client_send_message_pending returns true
and begin_send_message just calls red_channel_client_begin_send_message
as free_list->res->count is 0.

Should I rewrote this, maybe something like:

  Note that in dcc_send_item when an empty message is sent
  red_channel_client_send_message_pending always returns
  true and begin_send_message just calls red_channel_client_begin_send_message
  as free_list->res->count is 0.

> 
> > - 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).
> 
> I guess this could change the timing of receiving these messages? Ie
> they could be received a bit earlier than before? Probably not
> important.

If the loop message is good and the machine is not too busy
considering the network output buffer there should be no difference.
If there are probably the delay could even cause the kernel to
collapse some packets reducing slightly the bandwidth.
But we are probably taking about 50/100 us.

> There is still the "few difference" -> "little difference"
> 
> The patch itself looks good to me.
> 
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Frediano