Re: [Qemu-devel] [RFC] nbd: generalize usage of nbd_read

[Eric Blake]

On 1/16/19 8:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> We generally do very similar things around nbd_read: error_prepend,
> specifying, what we have tried to read and be_to_cpu conversion of
> integers.
> 
> So, it seems reasonable to move common things to helper functions,
> which:
> 1. simplify code a bit
> 2. generalize nbd_read error descriptions, all starting with
>    "Failed to read"
> 3. make it more difficult to forget to convert things from BE
> 
> Possible TODO:
> make a helper to read sized variable buffers, like
> 
>   uint32_t len
>   @len bytes buffer follows
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> 
> Hi Eric!
> 
> I have an idea of several helpers around nbd_read, what do you think
> about it?
> 
> It now based on your "nbd: add qemu-nbd --list", and if we like it,
> I'll fix iotests appropriately (if they fail, but I think, they should)
> and rebase onto final version of your series.
> 
> Based-on:<address@hidden>
> 
>  include/block/nbd.h | 33 ++++++++++++++++-
>  block/nbd-client.c  |  5 +--
>  nbd/client.c        | 89 ++++++++++++++-------------------------------
>  nbd/common.c        |  2 +-
>  nbd/server.c        | 27 +++++---------
>  5 files changed, 71 insertions(+), 85 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 80ee3cc997..f3d3ffc749 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -23,6 +23,7 @@
>  #include "qapi/qapi-types-block.h"
>  #include "io/channel-socket.h"
>  #include "crypto/tlscreds.h"
> +#include "qapi/error.h"
>  
>  /* Handshake phase structs - this struct is passed on the wire */
>  
> @@ -336,11 +337,39 @@ void nbd_server_start(SocketAddress *addr, const char 
> *tls_creds,
>   * Reads @size bytes from @ioc. Returns 0 on success.
>   */
>  static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
> -                           Error **errp)
> +                           const char *desc, Error **errp)
>  {
> -    return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
> +    int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
> +
> +    if (ret < 0) {
> +        if (desc) {
> +            error_prepend(errp, "Failed to read %s: ", desc);
> +        } else {
> +            error_prepend(errp, "Failed to read from socket: ");
> +        }

Do we need the error_prepend() at all when there is no desc?  For that
matter, does prepending "Failed to read..." add any useful information
to the error message in the first place?

> +        return -1;
> +    }
> +
> +    return 0;
>  }
>  
> +#define DEF_NBD_READ(bits) \
> +static inline int nbd_read ## bits(QIOChannel *ioc, uint ## bits ## _t *val, 
> \
> +                                   const char *desc, Error **errp)           
> \
> +{                                                                            
> \
> +    if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) {                  
> \
> +        return -1;                                                           
> \
> +    }                                                                        
> \
> +    *val = be ## bits ## _to_cpu(*val);                                      
> \
> +    return 0;                                                                
> \
> +}
> +
> +DEF_NBD_READ(16)
> +DEF_NBD_READ(32)
> +DEF_NBD_READ(64)

I'd write:

DEF_NBD_READ(16) /* nbd_read16 */

to aid grep-ability of the source code when looking where the function
is defined.

> +
> +#undef DEF_NBD_READ
> +
>  static inline bool nbd_reply_is_simple(NBDReply *reply)
>  {
>      return reply->magic == NBD_SIMPLE_REPLY_MAGIC;
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 813539676d..5c97052608 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -337,10 +337,9 @@ static int 
> nbd_co_receive_offset_data_payload(NBDClientSession *s,
>          return -EINVAL;
>      }
>  
> -    if (nbd_read(s->ioc, &offset, sizeof(offset), errp) < 0) {
> +    if (nbd_read64(s->ioc, &offset, "OFFSET_DATA offset", errp) < 0) {
>          return -EIO;
>      }
> -    be64_to_cpus(&offset);

The idea seems to be worthwhile; the callsites are indeed shorter when
using the new wrappers.

>  
>      data_size = chunk->length - sizeof(offset);
>      assert(data_size);
> @@ -387,7 +386,7 @@ static coroutine_fn int nbd_co_receive_structured_payload(
>      }
>  
>      *payload = g_new(char, len);
> -    ret = nbd_read(s->ioc, *payload, len, errp);
> +    ret = nbd_read(s->ioc, *payload, len, "structured payload", errp);

Okay, so maybe prepending DOES help, in letting you know which part of
the message was being read.


> @@ -166,10 +165,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>              goto cleanup;
>          }
>          msg = g_malloc(reply->length + 1);
> -        if (nbd_read(ioc, msg, reply->length, errp) < 0) {
> -            error_prepend(errp, "failed to read option error %" PRIu32
> -                          " (%s) message: ",
> -                          reply->type, nbd_rep_lookup(reply->type));
> +        if (nbd_read(ioc, msg, reply->length, "option error", errp) < 0) {

On the other hand, the text being prepended here loses some information
compared to what it previously had. If none of the iotests are
triggering this particular failure path, it's hard to argue that anyone
is hitting it enough to notice the difference, at which point is it even
worth prepending anything if the error is unlikely.

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

signature.asc
Description: OpenPGP digital signature