[Spice-devel] [spice-gtk v2 2/4] spice-channel: spice_channel_read_wire() improvements

[Victor Toso]

Hi,

On Tue, Feb 21, 2017 at 06:18:29PM +0100, Christophe Fergeau wrote:
> On Fri, Feb 03, 2017 at 04:13:38PM +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > * Removes the reread label by having while(TRUE);
> > 
> >   The label is being used after g_coroutine_socket_wait() is called,
> >   which is context switch while we can't do the reading as it would
> >   block. Moving this inside a while() makes the code more straight
> >   forward to be read and should not impact the performance.
> > 
> > * Move local variables inside the block they will be used.
> > 
> > Related: https://bugs.freedesktop.org/show_bug.cgi?id=96598
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/spice-channel.c | 38 ++++++++++++++++++++------------------
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > index a17c402..d1714df 100644
> > --- a/src/spice-channel.c
> > +++ b/src/spice-channel.c
> > @@ -1025,32 +1025,34 @@ static int spice_channel_read_wire_nonblocking(SpiceChannel *channel,
> >  static int spice_channel_read_wire(SpiceChannel *channel, void *data, size_t len)
> >  {
> >      SpiceChannelPrivate *c = channel->priv;
> > -    GIOCondition cond;
> > -    gssize ret;
> >  
> > -reread:
> > +    while (TRUE) {
> > +        gssize ret;
> > +        GIOCondition cond;
> >  
> > -    if (c->has_error) return 0; /* has_error is set by disconnect(), return no error */
> > +        if (c->has_error) {
> > +            /* has_error is set by disconnect(), return no error */
> > +            return 0;
> > +        }
> >  
> > -    ret = spice_channel_read_wire_nonblocking(channel, data, len, &cond);
> > +        ret = spice_channel_read_wire_nonblocking(channel, data, len, &cond);
> > +        if (ret > 0)
> > +            return ret;
> >
> > -    if (ret == -1) {
> > -        if (cond != 0) {
> > -            // TODO: should use g_pollable_input/output_stream_create_source() ?
> > -            g_coroutine_socket_wait(&c->coroutine, c->sock, cond);
> > -            goto reread;
> > -        } else {
> > +        if (ret == 0) {
> > +            CHANNEL_DEBUG(channel, "Closing the connection: spice_channel_read() - ret=0");
> > +            c->has_error = TRUE;
> > +            return 0;
> > +        }
> > +
> > +        if (cond == 0) {
> >              c->has_error = TRUE;
> >              return -errno;
> >          }
> > -    }
> > -    if (ret == 0) {
> > -        CHANNEL_DEBUG(channel, "Closing the connection: spice_channel_read() - ret=0");
> > -        c->has_error = TRUE;
> > -        return 0;
> > -    }
> >
> > -    return ret;
> > +        // TODO: should use g_pollable_input/output_stream_create_source() ?
> > +        g_coroutine_socket_wait(&c->coroutine, c->sock, cond);
> > +    }
>
> So basically what this change is doing is moving all the checks
> leading to early returns first, and then it handles the remaining
> case, which consists in waiting for data availability.

Yep

> As with patch 4/4, I regret that we change for toplevel tests checking
> only for 'ret' value to toplevel tests testing either 'ret' or 'cond'.
> I'd also add a bunch of 'else' while you are at it so that it's
> explicit that everything is mutually exclusive.

Okay, I'll rethink it and try to improve. It is true that checking for
ret before and cond afterwards is worse now then it was before but
if + else if + else should improve it.

> If I were writing these patches, I might even go as far as adding a
> first commit doing a switch to while(TRUE) and replacing the 'goto'
> with 'continue', and doing the reformatting, and then have another
> commit which moves around the various conditions.
>
> Christophe

Sure, I'll do that.
Many thanks for the reviews!

        toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170222/025a531e/attachment-0001.sig>