Re: [Qemu-block] [PATCH] nbd: Tolerate more errors to structured reply request

[Vladimir Sementsov-Ogievskiy]

19.08.2019 20:57, Eric Blake wrote:
> A server may have a reason to reject a request for structured replies,
> beyond just not recognizing them as a valid request.  It doesn't hurt
> us to continue talking to such a server; otherwise 'qemu-nbd --list'
> of such a server fails to display all possible details about the
> export.
> 
> Encountered when temporarily tweaking nbdkit to reply with
> NBD_REP_ERR_POLICY.  Present since structured reply support was first
> added (commit d795299b reused starttls handling, but starttls has to
> reject all errors).
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   nbd/client.c | 39 +++++++++++++++++++++++----------------
>   1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 8f524c3e3502..204f6e928d14 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,5 +1,5 @@
>   /*
> - *  Copyright (C) 2016-2018 Red Hat, Inc.
> + *  Copyright (C) 2016-2019 Red Hat, Inc.
>    *  Copyright (C) 2005  Anthony Liguori <address@hidden>
>    *
>    *  Network Block Device Client Side
> @@ -141,17 +141,19 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
> uint32_t opt,
>       return 0;
>   }
> 
> -/* If reply represents success, return 1 without further action.
> - * If reply represents an error, consume the optional payload of
> - * the packet on ioc.  Then return 0 for unsupported (so the client
> - * can fall back to other approaches), or -1 with errp set for other
> - * errors.
> +/*
> + * If reply represents success, return 1 without further action.  If
> + * reply represents an error, consume the optional payload of the
> + * packet on ioc.  Then return 0 for unsupported (so the client can
> + * fall back to other approaches), where @strict determines if only
> + * ERR_UNSUP or all errors fit that category, or -1 with errp set for

hmm not all but "errors returned by server accordingly to protocol", as "all"
a bit in contrast with following "result = -1", but it's OK as is anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> + * other errors.
>    */
>   static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> -                                Error **errp)
> +                                bool strict, Error **errp)
>   {
>       char *msg = NULL;
> -    int result = -1;
> +    int result = strict ? -1 : 0;
> 
>       if (!(reply->type & (1 << 31))) {
>           return 1;
> @@ -162,6 +164,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>               error_setg(errp, "server error %" PRIu32
>                          " (%s) message is too long",
>                          reply->type, nbd_rep_lookup(reply->type));
> +            result = -1;
>               goto cleanup;
>           }
>           msg = g_malloc(reply->length + 1);
> @@ -169,6 +172,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>               error_prepend(errp, "Failed to read option error %" PRIu32
>                             " (%s) message: ",
>                             reply->type, nbd_rep_lookup(reply->type));
> +            result = -1;
>               goto cleanup;
>           }
>           msg[reply->length] = '\0';
> @@ -257,7 +261,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
> char **description,
>       if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
>           return -1;
>       }
> -    error = nbd_handle_reply_err(ioc, &reply, errp);
> +    error = nbd_handle_reply_err(ioc, &reply, true, errp);
>       if (error <= 0) {
>           return error;
>       }
> @@ -370,7 +374,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t 
> opt,
>           if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
>               return -1;
>           }
> -        error = nbd_handle_reply_err(ioc, &reply, errp);
> +        error = nbd_handle_reply_err(ioc, &reply, true, errp);
>           if (error <= 0) {
>               return error;
>           }
> @@ -545,12 +549,15 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
>       }
>   }
> 
> -/* nbd_request_simple_option: Send an option request, and parse the reply
> +/*
> + * nbd_request_simple_option: Send an option request, and parse the reply.
> + * @strict controls whether ERR_UNSUP or all errors produce 0 status.
>    * return 1 for successful negotiation,
>    *        0 if operation is unsupported,
>    *        -1 with errp set for any other error
>    */
> -static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
> +static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
> +                                     Error **errp)
>   {
>       NBDOptionReply reply;
>       int error;
> @@ -562,7 +569,7 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
> opt, Error **errp)
>       if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
>           return -1;
>       }
> -    error = nbd_handle_reply_err(ioc, &reply, errp);
> +    error = nbd_handle_reply_err(ioc, &reply, strict, errp);
>       if (error <= 0) {
>           return error;
>       }
> @@ -594,7 +601,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>       QIOChannelTLS *tioc;
>       struct NBDTLSHandshakeData data = { 0 };
> 
> -    ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
> +    ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
>       if (ret <= 0) {
>           if (ret == 0) {
>               error_setg(errp, "Server don't support STARTTLS option");
> @@ -694,7 +701,7 @@ static int nbd_receive_one_meta_context(QIOChannel *ioc,
>           return -1;
>       }
> 
> -    ret = nbd_handle_reply_err(ioc, &reply, errp);
> +    ret = nbd_handle_reply_err(ioc, &reply, true, errp);
>       if (ret <= 0) {
>           return ret;
>       }
> @@ -950,7 +957,7 @@ static int nbd_start_negotiate(AioContext *aio_context, 
> QIOChannel *ioc,
>               if (structured_reply) {
>                   result = nbd_request_simple_option(ioc,
>                                                      NBD_OPT_STRUCTURED_REPLY,
> -                                                   errp);
> +                                                   false, errp);
>                   if (result < 0) {
>                       return -EINVAL;
>                   }
> 


-- 
Best regards,
Vladimir