[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply r
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request |
|
Date: |
Tue, 20 Aug 2019 08:56:43 +0000 |
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