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

[Eric Blake]

On 8/19/19 12:57 PM, 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(-)
> 

> -/* 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
> + * 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';

Previously - nbd_handle_reply_err() left errp unchanged when returning
0, now if strict=false and return is 0, errp may be set.

Doesn't affect callers that pass strict=true, but...


> -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;
>      }

> @@ -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;
>                  }

...this now can leave errp set, which can wreck callers.  I'll need to
post v2.

Also, I suspect that nbd_negotiate_simple_meta_context() should consider
the use of a non-strict error check (STARTTLS is really the only case
where if the server fails with an unexpected error, we really can't
continue on with some sane fallback regardless of the error).

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

signature.asc
Description: OpenPGP digital signature