[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST
From: |
Eric Blake |
Subject: |
[Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST |
Date: |
Fri, 8 Apr 2016 16:05:52 -0600 |
Since we know that the maximum name we are willing to accept
is small enough to stack-allocate, rework the iteration over
NBD_OPT_LIST responses to reuse a stack buffer rather than
allocating every time. Furthermore, we don't even have to
allocate if we know the server's length doesn't match what
we are searching for.
Not fixed here: Upstream NBD Protocol recently added this
clarification:
https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions
Where this document refers to a string, then unless otherwise
stated, that string is a sequence of UTF-8 code points, which
is not NUL terminated, MUST NOT contain NUL characters, SHOULD
be no longer than 256 bytes and MUST be no longer than 4096
bytes. This applies to export names and error messages (amongst
others).
To be fully compliant to that, we need to bump our export name
limit from 255 to at least 256, and need to decide whether we
can bump it higher (bumping it all the way to 4096 is annoying
in that we could no longer safely stack-allocate a worst-case
string, so we may still want to take the leeway offered by SHOULD
to force a reasonable smaller limit).
Signed-off-by: Eric Blake <address@hidden>
---
nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
1 file changed, 65 insertions(+), 65 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index b2dfc11..d4e37d5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -230,14 +230,17 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
nbd_opt_reply *reply,
return result;
}
-static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+/* Return -1 if unrecoverable error occurs, 0 if NBD_OPT_LIST is
+ * unsupported, 1 if iteration is done, 2 to keep looking, and 3 if
+ * this entry matches want. */
+static int nbd_receive_list(QIOChannel *ioc, const char *want, Error **errp)
{
nbd_opt_reply reply;
uint32_t len;
uint32_t namelen;
+ char name[NBD_MAX_NAME_SIZE + 1];
int error;
- *name = NULL;
if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
return -1;
}
@@ -252,97 +255,94 @@ static int nbd_receive_list(QIOChannel *ioc, char **name,
Error **errp)
error_setg(errp, "length too long for option end");
return -1;
}
- } else if (reply.type == NBD_REP_SERVER) {
- if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
- error_setg(errp, "incorrect option length %"PRIu32, len);
- return -1;
- }
- if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
- error_setg(errp, "failed to read option name length");
- return -1;
- }
- namelen = be32_to_cpu(namelen);
- len -= sizeof(namelen);
- if (len < namelen) {
- error_setg(errp, "incorrect option name length");
- return -1;
- }
- if (namelen > NBD_MAX_NAME_SIZE) {
- error_setg(errp, "export name length too long %" PRIu32, namelen);
- return -1;
- }
-
- *name = g_new0(char, namelen + 1);
- if (read_sync(ioc, *name, namelen) != namelen) {
- error_setg(errp, "failed to read export name");
- g_free(*name);
- *name = NULL;
- return -1;
- }
- (*name)[namelen] = '\0';
- len -= namelen;
- if (drop_sync(ioc, len) != len) {
- error_setg(errp, "failed to read export description");
- g_free(*name);
- *name = NULL;
- return -1;
- }
- } else {
+ return 1;
+ } else if (reply.type != NBD_REP_SERVER) {
error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
reply.type, NBD_REP_SERVER);
return -1;
}
- return 1;
+
+ if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
+ error_setg(errp, "incorrect option length %"PRIu32, len);
+ return -1;
+ }
+ if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+ error_setg(errp, "failed to read option name length");
+ return -1;
+ }
+ namelen = be32_to_cpu(namelen);
+ len -= sizeof(namelen);
+ if (len < namelen) {
+ error_setg(errp, "incorrect option name length");
+ return -1;
+ }
+ if (namelen != strlen(want)) {
+ if (drop_sync(ioc, len) != len) {
+ error_setg(errp, "failed to skip export name with wrong length");
+ return -1;
+ }
+ return 2;
+ }
+
+ assert(namelen < sizeof(name));
+ if (read_sync(ioc, name, namelen) != namelen) {
+ error_setg(errp, "failed to read export name");
+ return -1;
+ }
+ name[namelen] = '\0';
+ len -= namelen;
+ if (drop_sync(ioc, len) != len) {
+ error_setg(errp, "failed to read export description");
+ return -1;
+ }
+ return strcmp(name, want) == 0 ? 3 : 2;
}
+/* Return -1 on failure, 0 if wantname is an available export. */
static int nbd_receive_query_exports(QIOChannel *ioc,
const char *wantname,
Error **errp)
{
bool foundExport = false;
- TRACE("Querying export list");
+ TRACE("Querying export list for '%s'", wantname);
if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
return -1;
}
TRACE("Reading available export names");
while (1) {
- char *name = NULL;
- int ret = nbd_receive_list(ioc, &name, errp);
+ int ret = nbd_receive_list(ioc, wantname, errp);
- if (ret < 0) {
- g_free(name);
- name = NULL;
+ switch (ret) {
+ default:
+ /* Server gave unexpected reply */
+ assert(ret < 0);
return -1;
- }
- if (ret == 0) {
+ case 0:
/* Server doesn't support export listing, so
* we will just assume an export with our
* wanted name exists */
- foundExport = true;
- break;
- }
- if (name == NULL) {
- TRACE("End of export name list");
+ return 0;
+ case 1:
+ /* Done iterating. */
+ if (!foundExport) {
+ error_setg(errp, "No export with name '%s' available",
+ wantname);
+ return -1;
+ }
+ return 0;
+ case 2:
+ /* Wasn't this one, keep going. */
break;
- }
- if (g_str_equal(name, wantname)) {
+ case 3:
+ /* Found a match, but must finish parsing reply. */
+ TRACE("Found desired export name '%s'", wantname);
foundExport = true;
- TRACE("Found desired export name '%s'", name);
- } else {
- TRACE("Ignored export name '%s'", name);
+ break;
}
- g_free(name);
- }
-
- if (!foundExport) {
- error_setg(errp, "No export with name '%s' available", wantname);
- return -1;
}
-
- return 0;
}
static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
--
2.5.5
- [Qemu-devel] [PATCH 02/18] nbd: Don't fail handshake on NBD_OPT_LIST descriptions, (continued)
- [Qemu-devel] [PATCH 02/18] nbd: Don't fail handshake on NBD_OPT_LIST descriptions, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 14/18] nbd: Implement NBD_OPT_GO on client, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 10/18] nbd: Share common option-sending code in client, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 01/18] nbd: Don't kill server on client that doesn't request TLS, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 08/18] nbd: Limit nbdflags to 16 bits, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST,
Eric Blake <=
- [Qemu-devel] [PATCH 15/18] nbd: Implement NBD_OPT_GO on server, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 13/18] nbd: Support shorter handshake, Eric Blake, 2016/04/08
- [Qemu-devel] [PATCH 07/18] nbd: Treat flags vs. command type as separate fields, Eric Blake, 2016/04/08
- [Qemu-devel] [RFC PATCH 17/18] nbd: Implement NBD_CMD_WRITE_ZEROES on server, Eric Blake, 2016/04/08