Re: [Qemu-block] [PATCH 02/17] nbd/client: refactor nbd_read_eof

[Eric Blake]

On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Refactor nbd_read_eof to return 1 on success, 0 on eof, when no
> data was read and <0 for other cases, because returned size of
> read data is not actually used.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/nbd-internal.h         | 34 +++++++++++++++++++++++++---------
>  nbd/client.c               |  5 -----
>  tests/qemu-iotests/083.out |  4 ++--
>  3 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 396ddb4d3e..3fb0b6098a 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -77,21 +77,37 @@
>  #define NBD_ESHUTDOWN  108
>  
>  /* nbd_read_eof
> - * Tries to read @size bytes from @ioc. Returns number of bytes actually 
> read.
> - * May return a value >= 0 and < size only on EOF, i.e. when iteratively 
> called
> - * qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof
> - * iteratively.
> + * Tries to read @size bytes from @ioc.
> + * Returns 1 on success
> + *         0 on eof, when no data was read (errp is not set)
> + *         -EINVAL on eof, when some data < @size was read until eof
> + *         < 0 on read fail

In general, mixing negative errno value and generic < 0 in the same
function is most likely ambiguous.

>   */
> -static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t 
> size,
> -                                   Error **errp)
> +static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
> +                               Error **errp)
>  {
>      struct iovec iov = { .iov_base = buffer, .iov_len = size };
> +    ssize_t ret;
> +
>      /* Sockets are kept in blocking mode in the negotiation phase.  After
>       * that, a non-readable socket simply means that another thread stole
>       * our request/reply.  Synchronization is done with recv_coroutine, so
>       * that this is coroutine-safe.
>       */
> -    return nbd_rwv(ioc, &iov, 1, size, true, errp);
> +
> +    assert(size > 0);

Effectively the same as assert(size != 0).

> +
> +    ret = nbd_rwv(ioc, &iov, 1, size, true, errp);
> +    if (ret <= 0) {
> +        return ret;
> +    }

So this is a negative errno (or 0 on EOF),

> +
> +    if (ret != size) {
> +        error_setg(errp, "End of file");
> +        return -EINVAL;

and so is this. Which makes the function documentation not quite
accurate; you aren't mixing a generic < 0.

> +    }
> +
> +    return 1;
>  }
>  
>  /* nbd_read
> @@ -100,9 +116,9 @@ static inline ssize_t nbd_read_eof(QIOChannel *ioc, void 
> *buffer, size_t size,
>  static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
>                             Error **errp)
>  {
> -    ssize_t ret = nbd_read_eof(ioc, buffer, size, errp);
> +    int ret = nbd_read_eof(ioc, buffer, size, errp);
>  
> -    if (ret >= 0 && ret != size) {
> +    if (ret == 0) {
>          ret = -EINVAL;
>          error_setg(errp, "End of file");

Why do we have to set errp here instead of in nbd_read_eof()? Is there
ever any case where hitting early EOF is not something that should be
treated as an error?

>      }
> diff --git a/nbd/client.c b/nbd/client.c
> index f1c16b588f..4556056daa 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -925,11 +925,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply 
> *reply, Error **errp)
>          return ret;
>      }
>  
> -    if (ret != sizeof(buf)) {
> -        error_setg(errp, "read failed");
> -        return -EINVAL;
> -    }
> -
>      /* Reply
>         [ 0 ..  3]    magic   (NBD_REPLY_MAGIC)
>         [ 4 ..  7]    error   (0 == no error)
> diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
> index a24c6bfece..d3bea1b2f5 100644
> --- a/tests/qemu-iotests/083.out
> +++ b/tests/qemu-iotests/083.out
> @@ -69,12 +69,12 @@ read failed: Input/output error
>  
>  === Check disconnect 4 reply ===
>  
> -read failed
> +End of file
>  read failed: Input/output error

At least you tracked that your changes tweak the error message.  But I'm
not yet convinced whether this change simplifies anything.  Is there a
later patch that is easier to write with the new semantics which was not
possible with the pre-patch semantics?

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

signature.asc
Description: OpenPGP digital signature