[Spice-devel] [spice-gtk v2 4/4] spice-channel: make _flush_wire() compatible to _read_wire()

[Christophe Fergeau]

On Fri, Feb 03, 2017 at 04:13:40PM +0100, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> * Keeping compatibility with spice_channel_read_wire() where we keep
I would not talk about 'compatibility", just saying they are similar ?

>   the possible context switch of g_coroutine_socket_wait() in the end
>   of loop and dealing with the return of the _flush_wire_nonblocking()
>   case by case.
> 
> Related: https://bugs.freedesktop.org/show_bug.cgi?id=96598
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-channel.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 9e43c6c..55d3494 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -832,26 +832,28 @@ static void spice_channel_flush_wire(SpiceChannel *channel,
>      while (offset < datalen) {
>          gssize ret;
>  
> -        if (c->has_error) return;
> +        if (c->has_error)
> +            return;
>  
>          ret = spice_channel_flush_wire_nonblocking(channel, ptr+offset, datalen-offset, &cond);
> -        if (ret == -1) {
> -            if (cond != 0) {
> -                // TODO: should use g_pollable_input/output_stream_create_source() in 2.28 ?
> -                g_coroutine_socket_wait(&c->coroutine, c->sock, cond);
> -                continue;
> -            } else {
> -                CHANNEL_DEBUG(channel, "Closing the channel: spice_channel_flush %d", errno);
> -                c->has_error = TRUE;
> -                return;
> -            }
> +        if (ret > 0) {
> +            offset += ret;
> +            continue;
>          }
> +
>          if (ret == 0) {
>              CHANNEL_DEBUG(channel, "Closing the connection: spice_channel_flush");
>              c->has_error = TRUE;
>              return;
>          }
> -        offset += ret;
> +
> +        if (cond == 0) {

Fwiw, I think I find the initial version a bit easier to follow as all
the toplevel testing is checking 'ret' value, and then you look at other
things (cond).
One thing I'd add though is a bunch of else so that it's clear that the
various cases are exclusive.

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/10265a5f/attachment.sig>