* [Qemu-devel] [PATCH v2 1/2] nbd: Use g_autofree in a few places
2019-08-24 17:28 [Qemu-devel] [PATCH v2 0/2] nbd: tolerate more errors to extension requests Eric Blake
@ 2019-08-24 17:28 ` Eric Blake
2019-08-27 8:42 ` Daniel P. Berrangé
2019-08-27 13:51 ` Vladimir Sementsov-Ogievskiy
2019-08-24 17:28 ` [Qemu-devel] [PATCH v2 2/2] nbd: Tolerate more errors to structured reply request Eric Blake
2019-08-27 13:56 ` [Qemu-devel] [PATCH v2 0/2] nbd: tolerate more errors to extension requests Eric Blake
2 siblings, 2 replies; 7+ messages in thread
From: Eric Blake @ 2019-08-24 17:28 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, rjones, open list:Network Block Dev...,
Max Reitz
Thanks to our recent move to use glib's g_autofree, I can join the
bandwagon. Getting rid of gotos is fun ;)
There are probably more places where we could register cleanup
functions and get rid of more gotos; this patch just focuses on the
labels that existed merely to call g_free.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
block/nbd.c | 11 ++++-------
nbd/client.c | 22 +++++++---------------
nbd/server.c | 12 ++++--------
3 files changed, 15 insertions(+), 30 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index beed46fb3414..c4c91a158602 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1374,7 +1374,7 @@ static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
static void nbd_parse_filename(const char *filename, QDict *options,
Error **errp)
{
- char *file;
+ g_autofree char *file = NULL;
char *export_name;
const char *host_spec;
const char *unixpath;
@@ -1396,7 +1396,7 @@ static void nbd_parse_filename(const char *filename, QDict *options,
export_name = strstr(file, EN_OPTSTR);
if (export_name) {
if (export_name[strlen(EN_OPTSTR)] == 0) {
- goto out;
+ return;
}
export_name[0] = 0; /* truncate 'file' */
export_name += strlen(EN_OPTSTR);
@@ -1407,11 +1407,11 @@ static void nbd_parse_filename(const char *filename, QDict *options,
/* extract the host_spec - fail if it's not nbd:... */
if (!strstart(file, "nbd:", &host_spec)) {
error_setg(errp, "File name string for NBD must start with 'nbd:'");
- goto out;
+ return;
}
if (!*host_spec) {
- goto out;
+ return;
}
/* are we a UNIX or TCP socket? */
@@ -1431,9 +1431,6 @@ static void nbd_parse_filename(const char *filename, QDict *options,
out_inet:
qapi_free_InetSocketAddress(addr);
}
-
-out:
- g_free(file);
}
static bool nbd_process_legacy_socket_options(QDict *output_options,
diff --git a/nbd/client.c b/nbd/client.c
index 49bf9906f94b..a9d8d32feff7 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -247,12 +247,11 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
Error **errp)
{
- int ret = -1;
NBDOptionReply reply;
uint32_t len;
uint32_t namelen;
- char *local_name = NULL;
- char *local_desc = NULL;
+ g_autofree char *local_name = NULL;
+ g_autofree char *local_desc = NULL;
int error;
if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
@@ -298,7 +297,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
local_name = g_malloc(namelen + 1);
if (nbd_read(ioc, local_name, namelen, "export name", errp) < 0) {
nbd_send_opt_abort(ioc);
- goto out;
+ return -1;
}
local_name[namelen] = '\0';
len -= namelen;
@@ -306,24 +305,17 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
local_desc = g_malloc(len + 1);
if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) {
nbd_send_opt_abort(ioc);
- goto out;
+ return -1;
}
local_desc[len] = '\0';
}
trace_nbd_receive_list(local_name, local_desc ?: "");
- *name = local_name;
- local_name = NULL;
+ *name = g_steal_pointer(&local_name);
if (description) {
- *description = local_desc;
- local_desc = NULL;
+ *description = g_steal_pointer(&local_desc);
}
- ret = 1;
-
- out:
- g_free(local_name);
- g_free(local_desc);
- return ret;
+ return 1;
}
diff --git a/nbd/server.c b/nbd/server.c
index 0fb41c6c50ea..74d205812fee 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -206,7 +206,7 @@ static int GCC_FMT_ATTR(4, 0)
nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
Error **errp, const char *fmt, va_list va)
{
- char *msg;
+ g_autofree char *msg = NULL;
int ret;
size_t len;
@@ -216,18 +216,14 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
trace_nbd_negotiate_send_rep_err(msg);
ret = nbd_negotiate_send_rep_len(client, type, len, errp);
if (ret < 0) {
- goto out;
+ return ret;
}
if (nbd_write(client->ioc, msg, len, errp) < 0) {
error_prepend(errp, "write failed (error message): ");
- ret = -EIO;
- } else {
- ret = 0;
+ return -EIO;
}
-out:
- g_free(msg);
- return ret;
+ return 0;
}
/* Send an error reply.
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v2 2/2] nbd: Tolerate more errors to structured reply request
2019-08-24 17:28 [Qemu-devel] [PATCH v2 0/2] nbd: tolerate more errors to extension requests Eric Blake
2019-08-24 17:28 ` [Qemu-devel] [PATCH v2 1/2] nbd: Use g_autofree in a few places Eric Blake
@ 2019-08-24 17:28 ` Eric Blake
2019-08-27 8:47 ` Daniel P. Berrangé
2019-08-27 13:56 ` [Qemu-devel] [PATCH v2 0/2] nbd: tolerate more errors to extension requests Eric Blake
2 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2019-08-24 17:28 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; similarly, it may
have a reason for rejecting a request for a meta context. It doesn't
hurt us to continue talking to such a server; otherwise 'qemu-nbd
--list' of such a server fails to display all available 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 is
different in that we can't fall back to other behavior on any error).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
nbd/client.c | 63 +++++++++++++++++++++++++-----------------------
nbd/trace-events | 2 +-
2 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index a9d8d32feff7..b9dc829175f9 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
@@ -142,17 +142,18 @@ 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;
+ g_autofree char *msg = NULL;
if (!(reply->type & (1 << 31))) {
return 1;
@@ -163,26 +164,28 @@ 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));
- goto cleanup;
+ goto err;
}
msg = g_malloc(reply->length + 1);
if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {
error_prepend(errp, "Failed to read option error %" PRIu32
" (%s) message: ",
reply->type, nbd_rep_lookup(reply->type));
- goto cleanup;
+ goto err;
}
msg[reply->length] = '\0';
trace_nbd_server_error_msg(reply->type,
nbd_reply_type_lookup(reply->type), msg);
}
+ if (reply->type == NBD_REP_ERR_UNSUP || !strict) {
+ trace_nbd_reply_err_ignored(reply->option,
+ nbd_opt_lookup(reply->option),
+ reply->type, nbd_rep_lookup(reply->type));
+ return 0;
+ }
+
switch (reply->type) {
- case NBD_REP_ERR_UNSUP:
- trace_nbd_reply_err_unsup(reply->option, nbd_opt_lookup(reply->option));
- result = 0;
- goto cleanup;
-
case NBD_REP_ERR_POLICY:
error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
@@ -227,12 +230,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
error_append_hint(errp, "server reported: %s\n", msg);
}
- cleanup:
- g_free(msg);
- if (result < 0) {
- nbd_send_opt_abort(ioc);
- }
- return result;
+ err:
+ nbd_send_opt_abort(ioc);
+ return -1;
}
/* nbd_receive_list:
@@ -257,7 +257,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;
}
@@ -363,7 +363,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;
}
@@ -538,12 +538,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;
@@ -555,7 +558,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;
}
@@ -587,7 +590,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");
@@ -687,7 +690,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, false, errp);
if (ret <= 0) {
return ret;
}
@@ -943,7 +946,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;
}
diff --git a/nbd/trace-events b/nbd/trace-events
index 7ab6b3788cb2..f6cde967903a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -4,7 +4,7 @@
nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option request %" PRIu32" (%s), len %" PRIu32
nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIu32" (%s), type %" PRIu32" (%s), len %" PRIu32
nbd_server_error_msg(uint32_t err, const char *type, const char *msg) "server reported error 0x%" PRIx32 " (%s) with additional message: %s"
-nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIu32 " (%s), attempting fallback"
+nbd_reply_err_ignored(uint32_t option, const char *name, uint32_t reply, const char *reply_name) "server failed request %" PRIu32 " (%s) with error 0x%" PRIx32 " (%s), attempting fallback"
nbd_receive_list(const char *name, const char *desc) "export list includes '%s', description '%s'"
nbd_opt_info_go_start(const char *opt, const char *name) "Attempting %s for export '%s'"
nbd_opt_info_go_success(const char *opt) "Export is ready after %s request"
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread