From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56276) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoeXo-0001uo-NZ for qemu-devel@nongnu.org; Fri, 08 Apr 2016 18:06:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aoeXn-0007VA-Ef for qemu-devel@nongnu.org; Fri, 08 Apr 2016 18:06:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45531) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoeXn-0007Uy-7X for qemu-devel@nongnu.org; Fri, 08 Apr 2016 18:06:23 -0400 From: Eric Blake Date: Fri, 8 Apr 2016 16:05:52 -0600 Message-Id: <1460153158-21612-13-git-send-email-eblake@redhat.com> In-Reply-To: <1460153158-21612-1-git-send-email-eblake@redhat.com> References: <1460153158-21612-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: alex@alex.org.uk, Paolo Bonzini 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 --- 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