Re: [Qemu-devel] [PATCH 14/18] nbd: Implement NBD_OPT_GO on client

[Alex Bligh]

On 8 Apr 2016, at 23:05, Eric Blake <address@hidden> wrote:

> NBD_OPT_EXPORT_NAME is lousy: it doesn't have any sane error
> reporting.  Upstream NBD recently added NBD_OPT_GO as the

... as an experimental option for now, but hopefully this
should move it out the experimental section.

Thanks for doing this one.

> improved version of the option that does what we want: it
> reports sane errors on failures (including when a server
> requires TLS but does not have NBD_OPT_GO!), and on success
> it concludes with the same data as NBD_OPT_EXPORT_NAME sends.
> 
> Signed-off-by: Eric Blake <address@hidden>

Perhaps worth adding that although all servers that support
FixedNewstyle are meant to support (i.e. error but not disconnect on)
unsupported options, perhaps some don't (in which case they
are buggy and should be fixed). But just in case someone asks
'why is qemu no longer connecting to shonkynbd', a message in
the commit log might be useful.

Otherwise:

Reviewed-by: Alex Bligh <address@hidden>

> ---
> include/block/nbd.h |  1 +
> nbd/nbd-internal.h  |  7 +++++
> nbd/client.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 35c0ea3..d261dbc 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -88,6 +88,7 @@ typedef struct nbd_reply nbd_reply;
> #define NBD_REP_ERR_POLICY      ((UINT32_C(1) << 31) | 2) /* Server denied */
> #define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. 
> */
> #define NBD_REP_ERR_TLS_REQD    ((UINT32_C(1) << 31) | 5) /* TLS required */
> +#define NBD_REP_ERR_UNKNOWN     ((UINT32_C(1) << 31) | 6) /* Export unknown 
> */
> 
> /* Request flags, sent from client to server during transmission phase */
> #define NBD_CMD_FLAG_FUA        (1 << 0)
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index b78d249..ddba1d0 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -55,8 +55,13 @@
>  * https://github.com/yoe/nbd/blob/master/doc/proto.md
>  */
> 
> +/* Size of all NBD_OPT_*, without payload */
> #define NBD_REQUEST_SIZE        (4 + 2 + 2 + 8 + 8 + 4)
> +/* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
> #define NBD_REPLY_SIZE          (4 + 4 + 8)
> +/* Size of reply to NBD_OPT_EXPORT_NAME, without trailing zeroes */
> +#define NBD_FINAL_REPLY_SIZE    (8 + 2)
> +
> #define NBD_REQUEST_MAGIC       0x25609513
> #define NBD_REPLY_MAGIC         0x67446698
> #define NBD_OPTS_MAGIC          0x49484156454F5054LL
> @@ -80,6 +85,8 @@
> #define NBD_OPT_LIST            (3)
> #define NBD_OPT_PEEK_EXPORT     (4)
> #define NBD_OPT_STARTTLS        (5)
> +#define NBD_OPT_INFO            (6)
> +#define NBD_OPT_GO              (7)
> 
> /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
>  * but only a limited set of errno values is specified in the protocol.
> diff --git a/nbd/client.c b/nbd/client.c
> index 507ddc1..af17d4c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -215,6 +215,11 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> nbd_opt_reply *reply,
>                    reply->option);
>         break;
> 
> +    case NBD_REP_ERR_UNKNOWN:
> +        error_setg(errp, "Requested export not available for option %" 
> PRIx32,
> +                   reply->option);
> +        break;
> +
>     default:
>         error_setg(errp, "Unknown error code when asking for option %" PRIx32,
>                    reply->option);
> @@ -299,6 +304,67 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, Error **errp)
> }
> 
> 
> +/* Returns -1 if NBD_OPT_GO proves the export cannot be used, 0 if
> + * NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
> + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
> + * go (with the server data at the same point as it would be right
> + * after sending NBD_OPT_EXPORT_NAME). */
> +static int nbd_opt_go(QIOChannel *ioc, const char *wantname, Error **errp)
> +{
> +    nbd_opt_reply reply;
> +    uint32_t len;
> +    uint32_t namelen;
> +    int error;
> +    char buf[NBD_MAX_NAME_SIZE];
> +
> +    TRACE("Attempting NBD_OPT_GO for export '%s'", wantname);
> +    if (nbd_send_option_request(ioc, NBD_OPT_GO, -1, wantname, errp) < 0) {
> +        return -1;
> +    }
> +
> +    TRACE("Reading export info");
> +    if (nbd_receive_option_reply(ioc, NBD_OPT_GO, &reply, errp) < 0) {
> +        return -1;
> +    }
> +    error = nbd_handle_reply_err(ioc, &reply, errp);
> +    if (error <= 0) {
> +        return error;
> +    }
> +    len = reply.length;
> +
> +    if (reply.type != NBD_REP_SERVER) {
> +        error_setg(errp, "unexpected reply type %" PRIx32 ", expected %x",
> +                   reply.type, NBD_REP_SERVER);
> +        return -1;
> +    }
> +
> +    if (len < sizeof(namelen) + NBD_FINAL_REPLY_SIZE ||
> +        len > sizeof(namelen) + sizeof(buf) + NBD_FINAL_REPLY_SIZE) {
> +        error_setg(errp, "reply with %" PRIu32 " bytes is unexpected size",
> +                   len);
> +        return -1;
> +    }
> +
> +    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> +        error_setg(errp, "failed to read namelen");
> +        return -1;
> +    }
> +    be32_to_cpus(&namelen);
> +    if (len != sizeof(namelen) + namelen + NBD_FINAL_REPLY_SIZE) {
> +        error_setg(errp, "namelen %" PRIu32 " is unexpected size",
> +                   len);
> +        return -1;
> +    }
> +
> +    if (read_sync(ioc, buf, namelen) != namelen) {
> +        error_setg(errp, "failed to read name");
> +        return -1;
> +    }
> +
> +    TRACE("export is good to go");
> +    return 1;
> +}
> +
> /* Return -1 on failure, 0 if wantname is an available export. */
> static int nbd_receive_query_exports(QIOChannel *ioc,
>                                      const char *wantname,
> @@ -505,11 +571,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>             name = "";
>         }
>         if (fixedNewStyle) {
> +            int result;
> +
> +            /* Try NBD_OPT_GO first - if it works, we are done (it
> +             * also gives us a good message if the server requires
> +             * TLS).  If it is not available, fall back to
> +             * NBD_OPT_LIST for nicer error messages about a missing
> +             * export, then use NBD_OPT_EXPORT_NAME.  */
> +            result = nbd_opt_go(ioc, name, errp);
> +            if (result < 0) {
> +                goto fail;
> +            }
> +            if (result > 0) {
> +                zeroes = false;
> +                goto success;
> +            }
>             /* Check our desired export is present in the
>              * server export list. Since NBD_OPT_EXPORT_NAME
>              * cannot return an error message, running this
> -             * query gives us good error reporting if the
> -             * server required TLS
> +             * query gives us better error reporting if the
> +             * export name is not available.
>              */
>             if (nbd_receive_query_exports(ioc, name, errp) < 0) {
>                 goto fail;
> @@ -522,6 +603,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>         }
> 
>         /* Read the response */
> +    success:
>         if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
>             error_setg(errp, "Failed to read export length");
>             goto fail;
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh