Re: [PATCH v3 1/4] io/channel: introduce qio_channel_pread{v, }_all{, _eof}()

[Fabiano Rosas <farosas@suse.de>]

Junjie Cao <junjie.cao@intel.com> writes:

> qio_channel_pread() and qio_channel_preadv() perform a single
> positioned read and may return a short result.  Callers that need all
> bytes currently have to open-code a retry loop or simply treat a short
> read as an error.
>
> Introduce four new helpers following the existing read_all / readv_all
> pattern:
>
>   qio_channel_preadv_all_eof()  -- retry loop; returns 1 on success,
>                                     0 on clean EOF, -1 on error.
>   qio_channel_preadv_all()      -- wraps _eof; treats early EOF as
>                                     error; returns 0 / -1.
>   qio_channel_pread_all_eof()   -- single-buffer convenience wrapper
>                                     around preadv_all_eof().
>   qio_channel_pread_all()       -- single-buffer convenience wrapper
>                                     around preadv_all().
>
> These advance the file offset internally after each partial read.
> All four are marked coroutine_mixed_fn, consistent with the existing
> _all helpers.
>
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>  include/io/channel.h | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  io/channel.c         | 91 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 183 insertions(+)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 1b02350437..47af409ede 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -634,6 +634,98 @@ ssize_t qio_channel_preadv(QIOChannel *ioc, const struct iovec *iov,
>  ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
>                            off_t offset, Error **errp);
>  
> +/**
> + * qio_channel_preadv_all_eof:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @iov, possibly blocking or (if the channel is non-blocking)
> + * yielding from the current coroutine multiple times until the entire
> + * content is read.  If end-of-file occurs immediately it is not an
> + * error, but if it occurs after data has been read it will return
> + * an error rather than a short-read.  Otherwise behaves as
> + * qio_channel_preadv().
> + *
> + * Returns: 1 if all bytes were read, 0 if end-of-file occurs
> + *          without data, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
> +                                                  const struct iovec *iov,
> +                                                  size_t niov,
> +                                                  off_t offset,
> +                                                  Error **errp);
> +
> +/**
> + * qio_channel_preadv_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @iov, possibly blocking or (if the channel is non-blocking)
> + * yielding from the current coroutine multiple times until the entire
> + * content is read.  If end-of-file occurs before all requested data
> + * has been read, an error will be reported.  Otherwise behaves as
> + * qio_channel_preadv().
> + *
> + * Returns: 0 if all bytes were read, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
> +                                              const struct iovec *iov,
> +                                              size_t niov,
> +                                              off_t offset,
> +                                              Error **errp);
> +
> +/**
> + * qio_channel_pread_all_eof:
> + * @ioc: the channel object
> + * @buf: the memory region to read data into
> + * @buflen: the number of bytes to read into @buf
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @buflen bytes, possibly blocking or (if the channel is
> + * non-blocking) yielding from the current coroutine multiple times
> + * until the entire content is read.  If end-of-file occurs
> + * immediately it is not an error, but if it occurs after data has
> + * been read it will return an error rather than a short-read.
> + * Otherwise behaves as qio_channel_pread().
> + *
> + * Returns: 1 if all bytes were read, 0 if end-of-file occurs
> + *          without data, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_pread_all_eof(QIOChannel *ioc,
> +                                                 void *buf,
> +                                                 size_t buflen,
> +                                                 off_t offset,
> +                                                 Error **errp);
> +
> +/**
> + * qio_channel_pread_all:
> + * @ioc: the channel object
> + * @buf: the memory region to read data into
> + * @buflen: the number of bytes to read into @buf
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @buflen bytes, possibly blocking or (if the channel is
> + * non-blocking) yielding from the current coroutine multiple times
> + * until the entire content is read.  If end-of-file occurs before
> + * all requested data has been read, an error will be reported.
> + * Otherwise behaves as qio_channel_pread().
> + *
> + * Returns: 0 if all bytes were read, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
> +                                             void *buf,
> +                                             size_t buflen,
> +                                             off_t offset,
> +                                             Error **errp);
> +
>  /**
>   * qio_channel_shutdown:
>   * @ioc: the channel object
> diff --git a/io/channel.c b/io/channel.c
> index cc02d997a4..52c1abfcbc 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -507,6 +507,97 @@ ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
>      return qio_channel_preadv(ioc, &iov, 1, offset, errp);
>  }
>  
> +int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
> +                                                  const struct iovec *iov,
> +                                                  size_t niov,
> +                                                  off_t offset,
> +                                                  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;
> +    bool partial = false;
> +
> +    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_preadv(ioc, local_iov, nlocal_iov, offset, errp);
> +
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            qio_channel_wait_cond(ioc, G_IO_IN);
> +            continue;
> +        }
> +
> +        if (len == 0) {
> +            if (!partial) {
> +                ret = 0;
> +                goto cleanup;
> +            }
> +            error_setg(errp,
> +                       "Unexpected end-of-file before all data were read");
> +            goto cleanup;
> +        }
> +
> +        if (len < 0) {
> +            goto cleanup;
> +        }
> +
> +        partial = true;
> +        offset += len;
> +        iov_discard_front(&local_iov, &nlocal_iov, len);
> +    }
> +
> +    ret = 1;
> +
> + cleanup:
> +    g_free(local_iov_head);
> +    return ret;
> +}
> +
> +int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
> +                                              const struct iovec *iov,
> +                                              size_t niov,
> +                                              off_t offset,
> +                                              Error **errp)
> +{
> +    int ret = qio_channel_preadv_all_eof(ioc, iov, niov, offset, errp);
> +
> +    if (ret == 0) {
> +        error_setg(errp,
> +                   "Unexpected end-of-file before all data were read");
> +        return -1;
> +    }
> +    if (ret == 1) {
> +        return 0;
> +    }
> +
> +    return ret;
> +}
> +
> +int coroutine_mixed_fn qio_channel_pread_all_eof(QIOChannel *ioc,
> +                                                 void *buf,
> +                                                 size_t buflen,
> +                                                 off_t offset,
> +                                                 Error **errp)
> +{
> +    struct iovec iov = { .iov_base = buf, .iov_len = buflen };
> +    return qio_channel_preadv_all_eof(ioc, &iov, 1, offset, errp);
> +}
> +
> +int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
> +                                             void *buf,
> +                                             size_t buflen,
> +                                             off_t offset,
> +                                             Error **errp)
> +{
> +    struct iovec iov = { .iov_base = buf, .iov_len = buflen };
> +    return qio_channel_preadv_all(ioc, &iov, 1, offset, errp);
> +}
> +
>  int qio_channel_shutdown(QIOChannel *ioc,
>                           QIOChannelShutdown how,
>                           Error **errp)

Reviewed-by: Fabiano Rosas <farosas@suse.de>