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

[Christophe Fergeau]

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. 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170221/12488914/attachment.sig>