Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes

[Anthony Liguori <aliguori@us.ibm.com>]

Laszlo Ersek <lersek@redhat.com> writes:

> The g_io_channel_write_chars() documentation states,
>
>   bytes_written: The number of bytes written. This can be nonzero even if
>                  the return value is not G_IO_STATUS_NORMAL. [...]
>
> io_channel_send() could lose such bytes before.
>
> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to
> constant false whenever it was reached. When that condition actually held,
> it always led to -1 / EINVAL. This patch (almost) distinguishes
> G_IO_STATUS_EOF only when no bytes have been written, and then treats it
> as an error.

Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen
if bytes_written > 0.  I see what you mean by the comment but do you
have any reason to believe this happens in practice?

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qemu-char.c |   41 +++++++++++++++++++----------------------
>  1 files changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 800d6a6..c86ce4b 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -720,35 +720,32 @@ static GIOChannel *io_channel_from_socket(int fd)
>  
>  static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
>  {
> -    GIOStatus status;
> -    size_t offset;
> +    size_t offset = 0;
> +    GIOStatus status = G_IO_STATUS_NORMAL;
>  
> -    offset = 0;
> -    while (offset < len) {
> -        gsize bytes_written;
> +    while (offset < len && status == G_IO_STATUS_NORMAL) {
> +        gsize bytes_written = 0;
>  
>          status = g_io_channel_write_chars(fd, buf + offset, len - offset,
>                                            &bytes_written, NULL);
> -        if (status != G_IO_STATUS_NORMAL) {
> -            if (status == G_IO_STATUS_AGAIN) {
> -                /* If we've written any data, return a partial write. */
> -                if (offset) {
> -                    break;
> -                }
> -                errno = EAGAIN;
> -            } else {
> -                errno = EINVAL;
> -            }
> -
> -            return -1;
> -        } else if (status == G_IO_STATUS_EOF) {
> -            break;
> -        }
> -
>          offset += bytes_written;
>      }
>  
> -    return offset;
> +    if (offset > 0) {
> +        return offset;
> +    }
> +    switch (status) {
> +    case G_IO_STATUS_NORMAL:
> +        g_assert(len == 0);
> +        return 0;
> +    case G_IO_STATUS_AGAIN:
> +        errno = EAGAIN;
> +        return -1;
> +    default:
> +        break;
> +    }
> +    errno = EINVAL;
> +    return -1;
>  }
>  
>  #ifndef _WIN32
> -- 
> 1.7.1