* [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request
@ 2019-08-19 17:57 Eric Blake
2019-08-20 8:56 ` Vladimir Sementsov-Ogievskiy
2019-08-21 14:55 ` Eric Blake
0 siblings, 2 replies; 3+ messages in thread
From: Eric Blake @ 2019-08-19 17:57 UTC (permalink / raw)
To: qemu-devel; +Cc: vsementsov, rjones, open list:Network Block Dev...
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 <eblake@redhat.com>
---
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 <anthony@codemonkey.ws>
*
* 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
+ * 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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request
2019-08-19 17:57 [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request Eric Blake
@ 2019-08-20 8:56 ` Vladimir Sementsov-Ogievskiy
2019-08-21 14:55 ` Eric Blake
1 sibling, 0 replies; 3+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-20 8:56 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: rjones, open list:Network Block Dev...
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 <eblake@redhat.com>
> ---
> 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 <anthony@codemonkey.ws>
> *
> * 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 <vsementsov@virtuozzo.com>
> + * 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request
2019-08-19 17:57 [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request Eric Blake
2019-08-20 8:56 ` Vladimir Sementsov-Ogievskiy
@ 2019-08-21 14:55 ` Eric Blake
1 sibling, 0 replies; 3+ messages in thread
From: Eric Blake @ 2019-08-21 14:55 UTC (permalink / raw)
To: qemu-devel; +Cc: vsementsov, rjones, open list:Network Block Dev...
[-- Attachment #1.1: Type: text/plain, Size: 4456 bytes --]
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 <eblake@redhat.com>
> ---
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-21 14:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 17:57 [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request Eric Blake
2019-08-20 8:56 ` Vladimir Sementsov-Ogievskiy
2019-08-21 14:55 ` Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).