Re: [Qemu-devel] [PATCH 07/10] io: add qio_channel_read/write_all

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Aug 22, 2017 at 03:18:29PM +0200, Paolo Bonzini wrote:
> It is pretty common to read a fixed-size buffer from a socket.  Add a
> function that does this, either with multiple reads (on blocking sockets)
> or by yielding if called from a coroutine.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/io/channel.h | 36 ++++++++++++++++++++++++++++++++++-
>  io/channel.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 1 deletion(-)

This lacks test suite coverage.

Also this is more or less the same as code Juan has proposed too:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01536.html

Rather than have dualing patch series, I'll post an update that
addresses my concerns with both patches, so we can merge these
new APIs independantly of your / Juan's patch series.

> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index db9bb022a1..9cfb4d081f 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -299,7 +299,7 @@ ssize_t qio_channel_writev(QIOChannel *ioc,
>                             Error **errp);
>  
>  /**
> - * qio_channel_readv:
> + * qio_channel_read:
>   * @ioc: the channel object
>   * @buf: the memory region to read data into
>   * @buflen: the length of @buf
> @@ -315,6 +315,23 @@ ssize_t qio_channel_read(QIOChannel *ioc,
>                           Error **errp);
>  
>  /**
> + * qio_channel_read_all:
> + * @ioc: the channel object
> + * @buf: the memory region to read data into
> + * @buflen: the number of bytes to @buf
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @buflen bytes into @buf, possibly blocking or (if the
> + * channel is non-blocking) yielding from the current coroutine
> + * multiple times until the entire content is read.  Otherwise
> + * behaves as qio_channel_read().
> + */
> +ssize_t coroutine_fn qio_channel_read_all(QIOChannel *ioc,
> +                                          char *buf,
> +                                          size_t buflen,
> +                                          Error **errp);
> +
> +/**
>   * qio_channel_write:
>   * @ioc: the channel object
>   * @buf: the memory regions to send data from
> @@ -331,6 +348,23 @@ ssize_t qio_channel_write(QIOChannel *ioc,
>                            Error **errp);
>  
>  /**
> + * qio_channel_write_all:
> + * @ioc: the channel object
> + * @buf: the memory region to write data into
> + * @buflen: the number of bytes to @buf
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Writes @buflen bytes from @buf, possibly blocking or (if the
> + * channel is non-blocking) yielding from the current coroutine
> + * multiple times until the entire content is written.  Otherwise
> + * behaves as qio_channel_write().
> + */
> +ssize_t coroutine_fn qio_channel_write_all(QIOChannel *ioc,
> +                                           const char *buf,
> +                                           size_t buflen,
> +                                           Error **errp);
> +
> +/**
>   * qio_channel_set_blocking:
>   * @ioc: the channel object
>   * @enabled: the blocking flag state
> diff --git a/io/channel.c b/io/channel.c
> index 1cfb8b33a2..7ab3f4eede 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -113,6 +113,60 @@ ssize_t qio_channel_read(QIOChannel *ioc,
>  }
>  
>  
> +ssize_t qio_channel_read_all(QIOChannel *ioc,
> +                             char *buf,
> +                             size_t buflen,
> +                             Error **errp)
> +{
> +    ssize_t total = 0;
> +    while (buflen > 0) {
> +        ssize_t n_read = qio_channel_read(ioc, buf, buflen, errp);
> +
> +        if (n_read == QIO_CHANNEL_ERR_BLOCK) {
> +            assert(ioc->ctx);
> +            qio_channel_yield(ioc, G_IO_IN);
> +            continue;
> +        }
> +        if (n_read < 0) {
> +            return n_read;
> +        }
> +
> +        buf += n_read;
> +        total += n_read;
> +        buflen -= n_read;
> +    }

This busy-loops on EOF ie when n_read == 0.

> +
> +    return total;
> +}
> +
> +
> +ssize_t qio_channel_write_all(QIOChannel *ioc,
> +                              const char *buf,
> +                              size_t buflen,
> +                              Error **errp)
> +{
> +    ssize_t total = 0;
> +    while (buflen > 0) {
> +        ssize_t n_written = qio_channel_write(ioc, buf, buflen, errp);
> +
> +        if (n_written == QIO_CHANNEL_ERR_BLOCK) {
> +            assert(ioc->ctx);
> +            qio_channel_yield(ioc, G_IO_OUT);
> +            continue;
> +        }
> +        if (n_written < 0) {
> +            return n_written;
> +        }
> +
> +        buf += n_written;
> +        total += n_written;
> +        buflen -= n_written;
> +    }
> +
> +    return total;
> +}
> +
> +
>  ssize_t qio_channel_write(QIOChannel *ioc,
>                            const char *buf,
>                            size_t buflen,
> -- 
> 2.13.5
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|