[Spice-devel] [spice-server 2/3] Fix some small red_channel_client_msg_sent() regression

[Frediano Ziglio]

> 
> On Tue, 2017-04-11 at 11:58 +0200, Christophe Fergeau wrote:
> > Commit 0239dfa added a call to red_channel_client_clear_sent_item()
> > to
> > red_channel_client_msg_sent(). One of the thing that
> > red_channel_client_msg_sent() does is to reset rcc->priv-
> > >send_data.blocked
> > to FALSE.
> > 
> > This means that the preexisting check for this value in
> > red_channel_client_msg_sent() became dead code as it comes right
> > after
> > the call to red_channel_client_clear_sent_item():
> > 
> >     red_channel_client_clear_sent_item(rcc);
> >     if (red_channel_client_is_blocked(rcc)) {
> >         [...]
> >     }
> > 
> > This commit moves the red_channel_client_clear_sent_item(); right
> > after
> > this check in order to avoid checking for a value which will always
> > be
> > FALSE.
> 
> Interesting. How did you find this? Did it cause some problem that you
> observed?
> 
> Jonathon
> 
> 
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > ---
> >  server/red-channel-client.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > index f9054ea..0b04c96 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -639,13 +639,13 @@ static void
> > red_channel_client_msg_sent(RedChannelClient *rcc)
> >              close(fd);
> >      }
> >  
> > -    red_channel_client_clear_sent_item(rcc);
> >      if (red_channel_client_is_blocked(rcc)) {
> >          SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(rcc->priv->channel);
> >          rcc->priv->send_data.blocked = FALSE;
> >          core->watch_update_mask(core, rcc->priv->stream->watch,
> >                                  SPICE_WATCH_EVENT_READ);
> >      }
> > +    red_channel_client_clear_sent_item(rcc);
> >  
> >      if (red_channel_client_urgent_marshaller_is_active(rcc)) {
> >          red_channel_client_restore_main_sender(rcc);

Something in this code looks weird...
Beside resetting the blocked flags this code is disabling the WRITE
event from the network.
Note that this function get called when the entire message is written
to the network layer.
This is not causing a delay as if there are other pending data other
data are appended to the network till we get a EAGAIN (or error) and
in this case the event (WRITE) is added again. So, if the network
output buffer get some space we'll write data.
However... is this code needed here? We need to be called on WRITE
if there are pending data still to send but this function only consider
the blocking flag so it will reset the event event if there are other
pending data in the queue (that is other messages to be sent). I
think this is the reason why the dead code worked too... there's no
need to reset the WRITE event or clear the blocked flag here!
Note the the WRITE event is reset also in red_channel_client_push
that is where data are sent and this function is called when a WRITE
event is received (so would be reset by this function in any case).

Frediano