On Fri, Feb 03, 2017 at 04:13:38PM +0100, Victor Toso wrote: > From: Victor Toso <me@xxxxxxxxxxxxxx> > > * 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@xxxxxxxxxx> > --- > 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. 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. 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel