Re: [Qemu-devel] [PATCH v2] io: add new qio_channel_{readv, writev, read, write}_all functions

[Eric Blake]

On 08/31/2017 04:46 AM, Daniel P. Berrange wrote:
> These functions wait until they are able to read / write the full
> requested data buffer(s).
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> 
> Changed in v2:
> 
>  - Remove 'coroutine_fn' annotation (Stefan)
>  - Fix docs typos (Eric)
>  - Remove bogus error overwriting (Eric)
> 
>  include/io/channel.h       |  90 +++++++++++++++++++++++++++++++++++++++
>  io/channel.c               |  94 +++++++++++++++++++++++++++++++++++++++++
>  tests/io-channel-helpers.c | 102 
> ++++-----------------------------------------
>  3 files changed, 193 insertions(+), 93 deletions(-)

Already applied, but just now noticing two things:

> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 54f3dc252f..8f25893c45 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -269,6 +269,58 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>                                  Error **errp);
>  
>  /**
> + * qio_channel_readv_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Read data from the IO channel, storing it in the
> + * memory regions referenced by @iov. Each element
> + * in the @iov will be fully populated with data
> + * before the next one is used. The @niov parameter
> + * specifies the total number of elements in @iov.
> + *
> + * The function will wait for all requested data
> + * to be read, yielding from the current coroutine
> + * if required.

[1] This states we yield...

> + *
> + * If end-of-file occurs before all requested data
> + * has been read, an error will be reported.

[2] nbd_read_eof() can't use this function, because it wants to
distinguish between early EOF (no data at all - server quit at a sane
point in the protocol, so the client doesn't need to report an error but
just start clean shutdown) and short read (EOF encountered after at
least one byte was read - server quit mid-message and client must report
the error).  But we don't want all callers to have to check for this
corner-case.  Obvious solution: add qio_channel_readv_all_eof() (I've
got the patch ready to submit, and can take it through my NBD tree since
I'm also patch nbd to take advantage of these new functions).

> +int qio_channel_readv_all(QIOChannel *ioc,

> + *
> + * The function will wait for all requested data
> + * to be written, yielding from the current coroutine
> + * if required.

[1] again

> +++ b/io/channel.c
> @@ -22,6 +22,7 @@
>  #include "io/channel.h"
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> +#include "qemu/iov.h"
>  
>  bool qio_channel_has_feature(QIOChannel *ioc,
>                               QIOChannelFeature feature)
> @@ -85,6 +86,79 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>  }
>  
>  
> +
> +int qio_channel_readv_all(QIOChannel *ioc,
> +                          const struct iovec *iov,
> +                          size_t niov,
> +                          Error **errp)
> +{
> +    int ret = -1;
> +    struct iovec *local_iov = g_new(struct iovec, niov);
> +    struct iovec *local_iov_head = local_iov;
> +    unsigned int nlocal_iov = niov;
> +
> +    nlocal_iov = iov_copy(local_iov, nlocal_iov,
> +                          iov, niov,
> +                          0, iov_size(iov, niov));
> +
> +    while (nlocal_iov > 0) {
> +        ssize_t len;
> +        len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            qio_channel_wait(ioc, G_IO_IN);

[1] Wait a minute. qio_channel_wait() spawns a NEW coroutine, rather
than yielding the current coroutine. nbd_rwv() was using
qio_channel_yield() here (after asserting that QIO_CHANNEL_ERR_BLOCK is
only possible for a non-blocking channel).  Keeping the wait in place
breaks iotests (things hang), while s/wait/yield/ lets NBD get through
iotests, but then breaks check-tests/test-io/channel-socket.  So I think
we have to make the code choose between the two conditions according to
whether it is currently in a coroutine.  I'll propose that patch, and
see what you say about it :)

> +            continue;
> +        } else if (len < 0) {
> +            goto cleanup;
> +        } else if (len == 0) {
> +            error_setg(errp,
> +                       "Unexpected end-of-file before all bytes were read");

[2] This is where the special-casing for early EOF vs. short read fits.


> +int qio_channel_writev_all(QIOChannel *ioc,
> +                           const struct iovec *iov,
> +                           size_t niov,
> +                           Error **errp)
> +{
> +    int ret = -1;
> +    struct iovec *local_iov = g_new(struct iovec, niov);
> +    struct iovec *local_iov_head = local_iov;
> +    unsigned int nlocal_iov = niov;
> +
> +    nlocal_iov = iov_copy(local_iov, nlocal_iov,
> +                          iov, niov,
> +                          0, iov_size(iov, niov));
> +
> +    while (nlocal_iov > 0) {
> +        ssize_t len;
> +        len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            qio_channel_wait(ioc, G_IO_OUT);

[1] again

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature