Re: [PATCH v20 08/20] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

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

On Tue, Jan 19, 2021 at 03:28:25PM -0500, Jagannathan Raman wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Adds qio_channel_readv_full_all_eof() and qio_channel_readv_full_all()
> to read both data and FDs. Refactors existing code to use these helpers.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  include/io/channel.h |  53 ++++++++++++++++++++++++++
>  io/channel.c         | 106 +++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 138 insertions(+), 21 deletions(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 19e76fc..8898897 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -778,6 +778,59 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
>                                      void *opaque);
>  
>  /**
> + * qio_channel_readv_full_all_eof:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data to
> + * @niov: the length of the @iov array
> + * @fds: an array of file handles to read
> + * @nfds: number of file handles in @fds
> + * @errp: pointer to a NULL-initialized error object
> + *
> + *
> + * Performs same function as qio_channel_readv_all_eof.
> + * Additionally, attempts to read file descriptors shared
> + * over the channel. The function will wait for all
> + * requested data to be read, yielding from the current
> + * coroutine if required. data refers to both file
> + * descriptors and the iovs.
> + *
> + * Returns: 1 if all bytes were read, 0 if end-of-file
> + *          occurs without data, or -1 on error
> + */
> +
> +int qio_channel_readv_full_all_eof(QIOChannel *ioc,
> +                                   const struct iovec *iov,
> +                                   size_t niov,
> +                                   int **fds, size_t *nfds,
> +                                   Error **errp);
> +
> +/**
> + * qio_channel_readv_full_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data to
> + * @niov: the length of the @iov array
> + * @fds: an array of file handles to read
> + * @nfds: number of file handles in @fds
> + * @errp: pointer to a NULL-initialized error object
> + *
> + *
> + * Performs same function as qio_channel_readv_all_eof.
> + * Additionally, attempts to read file descriptors shared
> + * over the channel. The function will wait for all
> + * requested data to be read, yielding from the current
> + * coroutine if required. data refers to both file
> + * descriptors and the iovs.
> + *
> + * Returns: 0 if all bytes were read, or -1 on error
> + */
> +
> +int qio_channel_readv_full_all(QIOChannel *ioc,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               int **fds, size_t *nfds,
> +                               Error **errp);
> +
> +/**
>   * qio_channel_writev_full_all:
>   * @ioc: the channel object
>   * @iov: the array of memory regions to write data from
> diff --git a/io/channel.c b/io/channel.c
> index 0d4b8b5..b12db9d 100644
> --- a/io/channel.c
> +++ b/io/channel.c


> +int qio_channel_readv_full_all_eof(QIOChannel *ioc,
> +                                   const struct iovec *iov,
> +                                   size_t niov,
> +                                   int **fds, size_t *nfds,
> +                                   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;
> +    int **local_fds = fds;
> +    size_t *local_nfds = nfds;
>      bool partial = false;
>  
> +    if (nfds) {
> +        *nfds = 0;
> +    }
> +
> +    if (fds) {
> +        *fds = NULL;
> +    }
> +
>      nlocal_iov = iov_copy(local_iov, nlocal_iov,
>                            iov, niov,
>                            0, iov_size(iov, niov));
>  
> -    while (nlocal_iov > 0) {
> +    while ((nlocal_iov > 0) || local_fds) {
>          ssize_t len;
> -        len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
> +        len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds,
> +                                     local_nfds, errp);
>          if (len == QIO_CHANNEL_ERR_BLOCK) {
>              if (qemu_in_coroutine()) {
>                  qio_channel_yield(ioc, G_IO_IN);
> @@ -112,20 +140,53 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>                  qio_channel_wait(ioc, G_IO_IN);
>              }
>              continue;
> -        } else if (len < 0) {
> -            goto cleanup;
> -        } else if (len == 0) {
> -            if (partial) {
> -                error_setg(errp,
> -                           "Unexpected end-of-file before all bytes were read");
> -            } else {
> -                ret = 0;
> +        }
> +
> +        if (len <= 0) {
> +            size_t fd_idx;
> +
> +            if (!len && !niov && (nfds && *nfds)) {
> +                break;
> +            }
> +
> +            if (!partial && (!nfds || !(*nfds))) {
> +                ret = len;
> +                goto cleanup;
>              }
> +
> +            error_prepend(errp,
> +                          "Unexpected end-of-file before all data were read.");
> +
> +            if (!nfds || !(*nfds)) {
> +                goto cleanup;
> +            }

I'm finding it really hard to understand what scenario each of
these three if() tests is validating, so can't be confident that
we've dealt with the failure cases correctly.

In the  len < 0 case, we shouldn't be reporting the "unexpected end-of-file"
message at all, as it won't be and end of file - its some other failure
condition.

In the len == 0 case, we should be using error_setg not error_prepend
AFAIK.

> +
> +            /*
> +             * If (len < 0) and fds are returned, it's not clear if the
> +             * returned fds are valid to be closed. Just free'ing the
> +             * allocated memory for fds in this case
> +             */
> +            fd_idx = *nfds;
> +            while (fd_idx && !len) {
> +                close((*fds)[fd_idx - 1]);
> +                fd_idx--;
> +            }

I'm not sure ignoring the len < 0 case is correct. The first time we
call qio_channel_readv_full(), we can receive some FDs, either with
or without some data bytes.

The second time we call qio_channel_readv_full we can get len == -1
and so need to return an error. We must close the FDs we received on
the previous iteration at this point.



> +
> +            g_free(*fds);
> +            *fds = NULL;
> +            *nfds = 0;
> +
>              goto cleanup;
>          }

I'm thinking the above error handling would be clearer if we could
separate out the len == 0 case from the len == -1 case initially.

eg, would this logic do what we need:

        if (len == 0) {
            if (local_nfds && *local_nfds) {
                /* got some FDs, but not data yet. This isn't an EOF
                 * scenario (yet), so carry on to try to read data
                 * on next loop iteration */
            } else if (!partial) {
                /* no fds and no data, must be an expected EOF */
                ret = 0;
                goto cleanup;
            } else {
                len = -1;
                error_setg(errp,
                           "Unexpected end-of-file before all data were read.");
                /* fallthrough into len < 0 handling now */
            }
        }

        if (len < 0) {
            /* Close any FDs we previously received */
            if (nfds && fds) {
                size_t i;
                for (i = 0; nfds && i < *nfds; i++) {
                    close((*fds)[i]);
                }

                g_free(*fds);
                *fds = NULL;
                *nfds = 0;
            }

            goto cleanup;
        }

>  
>          partial = true;
> -        iov_discard_front(&local_iov, &nlocal_iov, len);
> +
> +        if (nlocal_iov) {
> +            iov_discard_front(&local_iov, &nlocal_iov, len);
> +        }
> +
> +        local_fds = NULL;
> +        local_nfds = NULL;
>      }
>  
>      ret = 1;
> @@ -135,20 +196,23 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>      return ret;
>  }
>  
> -int qio_channel_readv_all(QIOChannel *ioc,
> -                          const struct iovec *iov,
> -                          size_t niov,
> -                          Error **errp)
> +int qio_channel_readv_full_all(QIOChannel *ioc,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               int **fds, size_t *nfds,
> +                               Error **errp)
>  {
> -    int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
> +    int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, errp);
>  
>      if (ret == 0) {
> -        ret = -1;
> -        error_setg(errp,
> -                   "Unexpected end-of-file before all bytes were read");
> -    } else if (ret == 1) {
> -        ret = 0;
> +        error_prepend(errp,
> +                      "Unexpected end-of-file before all data were read.");
> +        return -1;
>      }
> +    if (ret == 1) {
> +        return 0;
> +    }
> +
>      return ret;
>  }
>  
> -- 
> 1.8.3.1
> 

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 :|