| Message ID | 20190819175751.18075-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: Tolerate more errors to structured reply request | expand |
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; > } >
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).
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; }
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(-)