| Message ID | 20200925203249.155705-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
25.09.2020 23:32, Eric Blake wrote: > We had a premature optimization of trying to read as little from the > wire as possible while handling NBD_OPT_SET_META_CONTEXT in phases. > But in reality, we HAVE to read the entire string from the client > before we can get to the next command, and it is easier to just read > it all at once than it is to read it in pieces. And once we do that, > several functions end up no longer performing I/O, and no longer need > to return a value. > > While simplifying this, take advantage of g_str_has_prefix for less > repetition of boilerplate string length computation. > > Our iotests still pass; I also checked that libnbd's testsuite (which > covers more corner cases of odd meta context requests) still passes. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > nbd/server.c | 172 ++++++++++++++------------------------------------- > 1 file changed, 47 insertions(+), 125 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 982de67816a7..0d2d7e52058f 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2016-2018 Red Hat, Inc. > + * Copyright (C) 2016-2020 Red Hat, Inc. > * Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws> > * > * Network Block Device Server Side > @@ -792,135 +792,64 @@ static int nbd_negotiate_send_meta_context(NBDClient *client, > return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0; > } > > -/* Read strlen(@pattern) bytes, and set @match to true if they match @pattern. > - * @match is never set to false. > - * > - * Return -errno on I/O error, 0 if option was completely handled by > - * sending a reply about inconsistent lengths, or 1 on success. > - * > - * Note: return code = 1 doesn't mean that we've read exactly @pattern. > - * It only means that there are no errors. > + > +/* > + * Check @ns with @len bytes, and set @match to true if it matches @pattern, > + * or if @len is 0 and the client is performing _LIST_. @match is never set > + * to false. > */ > -static int nbd_meta_pattern(NBDClient *client, const char *pattern, bool *match, > - Error **errp) > +static void nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern, > + const char *ns, uint32_t len, ns changed its meaning, it's not just a namespace, but the whole query. I think, better to rename it. Also, it's unusual to pass length together with nul-terminated string, it seems redundant. And, it's used only to compare with zero, strlen(ns) == 0 or ns[0] == 0 is not slower. Also, errp is unused argument. And it violate Error API recommendation to not create void functions with errp. Also we can use bool return instead of return through pointer. > + bool *match, Error **errp) > { > - int ret; > - char *query; > - size_t len = strlen(pattern); > - > - assert(len); > - > - query = g_malloc(len); > - ret = nbd_opt_read(client, query, len, errp); > - if (ret <= 0) { > - g_free(query); > - return ret; > - } > - > - if (strncmp(query, pattern, len) == 0) { > + if (len == 0) { > + if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > + *match = true; > + } > + trace_nbd_negotiate_meta_query_parse("empty"); > + } else if (strcmp(pattern, ns) == 0) { > trace_nbd_negotiate_meta_query_parse(pattern); > *match = true; > } else { > trace_nbd_negotiate_meta_query_skip("pattern not matched"); > } > - g_free(query); > - > - return 1; > -} > - > -/* > - * Read @len bytes, and set @match to true if they match @pattern, or if @len > - * is 0 and the client is performing _LIST_. @match is never set to false. > - * > - * Return -errno on I/O error, 0 if option was completely handled by > - * sending a reply about inconsistent lengths, or 1 on success. > - * > - * Note: return code = 1 doesn't mean that we've read exactly @pattern. > - * It only means that there are no errors. > - */ > -static int nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern, > - uint32_t len, bool *match, Error **errp) > -{ > - if (len == 0) { > - if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > - *match = true; > - } > - trace_nbd_negotiate_meta_query_parse("empty"); > - return 1; > - } > - > - if (len != strlen(pattern)) { > - trace_nbd_negotiate_meta_query_skip("different lengths"); > - return nbd_opt_skip(client, len, errp); > - } > - > - return nbd_meta_pattern(client, pattern, match, errp); > } > > /* nbd_meta_base_query > * > * Handle queries to 'base' namespace. For now, only the base:allocation > - * context is available. 'len' is the amount of text remaining to be read from > - * the current name, after the 'base:' portion has been stripped. > - * > - * Return -errno on I/O error, 0 if option was completely handled by > - * sending a reply about inconsistent lengths, or 1 on success. > + * context is available. @len is the length of @ns, including the 'base:' > + * prefix. > */ > -static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta, > - uint32_t len, Error **errp) > +static void nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta, > + const char *ns, uint32_t len, Error **errp) > { > - return nbd_meta_empty_or_pattern(client, "allocation", len, > - &meta->base_allocation, errp); > + nbd_meta_empty_or_pattern(client, "allocation", ns + 5, len - 5, This one is correct, but would be simpler, if only subquery (after colon) passed here. (Really, no reason to pass 'base:' to _base_ func) Hmm, I see that it gives a possibility to use meta->exp->export_bitmap_context directly. > + &meta->base_allocation, errp); > } > > /* nbd_meta_bitmap_query > * > * Handle query to 'qemu:' namespace. > - * @len is the amount of text remaining to be read from the current name, after > - * the 'qemu:' portion has been stripped. > - * > - * Return -errno on I/O error, 0 if option was completely handled by > - * sending a reply about inconsistent lengths, or 1 on success. */ > -static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, > - uint32_t len, Error **errp) > + * @len is the length of @ns, including the `qemu:' prefix. > + */ > +static void nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, > + const char *ns, uint32_t len, Error **errp) > { > - bool dirty_bitmap = false; > - size_t dirty_bitmap_len = strlen("dirty-bitmap:"); > - int ret; > - > if (!meta->exp->export_bitmap) { > trace_nbd_negotiate_meta_query_skip("no dirty-bitmap exported"); > - return nbd_opt_skip(client, len, errp); > - } > - > - if (len == 0) { > + } else if (len == 0) { s/0/5/ ? > if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > meta->bitmap = true; > } > trace_nbd_negotiate_meta_query_parse("empty"); > - return 1; > - } > - > - if (len < dirty_bitmap_len) { > - trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); > - return nbd_opt_skip(client, len, errp); > - } > - > - len -= dirty_bitmap_len; > - ret = nbd_meta_pattern(client, "dirty-bitmap:", &dirty_bitmap, errp); > - if (ret <= 0) { > - return ret; > - } > - if (!dirty_bitmap) { > + } else if (!g_str_has_prefix(ns + 5, "dirty-bitmap:")) { > trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); > - return nbd_opt_skip(client, len, errp); > + } else { > + trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); > + nbd_meta_empty_or_pattern(client, meta->exp->export_bitmap_context, > + ns, len, &meta->bitmap, errp); hmm. _empty_ is impossible here, as ns includes "qemu:dirty-bitmap".. > } > - > - trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); > - > - return nbd_meta_empty_or_pattern( > - client, meta->exp->export_bitmap_context + > - strlen("qemu:dirty_bitmap:"), len, &meta->bitmap, errp); > } > > /* nbd_negotiate_meta_query > @@ -930,22 +859,13 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, > * > * The only supported namespaces are 'base' and 'qemu'. > * > - * The function aims not wasting time and memory to read long unknown namespace > - * names. > - * > * Return -errno on I/O error, 0 if option was completely handled by > * sending a reply about inconsistent lengths, or 1 on success. */ > static int nbd_negotiate_meta_query(NBDClient *client, > NBDExportMetaContexts *meta, Error **errp) > { > - /* > - * Both 'qemu' and 'base' namespaces have length = 5 including a > - * colon. If another length namespace is later introduced, this > - * should certainly be refactored. > - */ > int ret; > - size_t ns_len = 5; > - char ns[5]; > + g_autofree char *ns = NULL; > uint32_t len; > > ret = nbd_opt_read(client, &len, sizeof(len), errp); > @@ -958,27 +878,29 @@ static int nbd_negotiate_meta_query(NBDClient *client, > trace_nbd_negotiate_meta_query_skip("length too long"); > return nbd_opt_skip(client, len, errp); > } > - if (len < ns_len) { > - trace_nbd_negotiate_meta_query_skip("length too short"); > - return nbd_opt_skip(client, len, errp); > - } > > - len -= ns_len; > - ret = nbd_opt_read(client, ns, ns_len, errp); > + ns = g_malloc(len + 1); > + ret = nbd_opt_read(client, ns, len, errp); Now ns is not a namespace but the whole query. I'd rename a variable. > if (ret <= 0) { > return ret; > } > + ns[len] = '\0'; > + if (strlen(ns) != len) { > + return nbd_opt_invalid(client, errp, > + "Embedded NUL in query for option %s", > + nbd_opt_lookup(client->opt)); > + } Hmm, that's a new good check. Intersting, is it specified in NBD protocol, that NUL shouldn't be in a string.. Probably it can be a separate patch, with new nbd_opt_string_read, which will check this thing. But I'm OK with it as is in this patch. > > - if (!strncmp(ns, "base:", ns_len)) { > + if (g_str_has_prefix(ns, "base:")) { > trace_nbd_negotiate_meta_query_parse("base:"); > - return nbd_meta_base_query(client, meta, len, errp); > - } else if (!strncmp(ns, "qemu:", ns_len)) { > + nbd_meta_base_query(client, meta, ns, len, errp); > + } else if (g_str_has_prefix(ns, "qemu:")) { > trace_nbd_negotiate_meta_query_parse("qemu:"); > - return nbd_meta_qemu_query(client, meta, len, errp); > + nbd_meta_qemu_query(client, meta, ns, len, errp); passing length of string together with string seems redundant (and error prone). > + } else { > + trace_nbd_negotiate_meta_query_skip("unknown namespace"); > } > - Seems you don't like my new-line before final return statement. > - trace_nbd_negotiate_meta_query_skip("unknown namespace"); > - return nbd_opt_skip(client, len, errp); > + return 1; > } > > /* nbd_negotiate_meta_queries > To avoid all this pointer arithmetic, what about something like this (I didn't check, just an idea): /* * Return true if @query matches @pattern of if @query is empty and the client * is performing _LIST_. Otherwise return false. */ static bool nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern, const char *query) { if (!strcmp(query, "")) { trace_nbd_negotiate_meta_query_parse("empty"); return client->opt == NBD_OPT_LIST_META_CONTEXT; } else if (!strcmp(query, pattern)) { trace_nbd_negotiate_meta_query_parse(pattern); return true; } else { trace_nbd_negotiate_meta_query_skip("pattern not matched"); return false; } } bool strshift(const char **str, const char *prefix) { if (g_str_has_prefix(*str, prefix)) { *str += strlen(prefix); return true; } return false; } /* * nbd_meta_base_query * * If it's 'base' namespace parse the query and return true. If not return * false. */ static bool nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta, const char *query) { if (!strshift(&query, "base:")) { return false; } trace_nbd_negotiate_meta_query_parse("base:"); meta->base_allocation = nbd_meta_empty_or_pattern(client, "allocation", query); return true; } /* * nbd_meta_bitmap_query * * If it's 'qemu' namespace parse the query and return true. If not return * false. */ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, const char *query) { if (!strshift(&query, "qemu:")) { return false; } trace_nbd_negotiate_meta_query_parse("qemu:"); if (!meta->exp->export_bitmap) { trace_nbd_negotiate_meta_query_skip("no dirty-bitmap exported"); return true; } if (!query[0]) { if (client->opt == NBD_OPT_LIST_META_CONTEXT) { meta->bitmap = true; } trace_nbd_negotiate_meta_query_parse("empty"); return true; } if (!strshift(&query, "dirty-bitmap:")) { trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); return true; } trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); meta->bitmap = nbd_meta_empty_or_pattern( client, meta->exp->export_bitmap_context + strlen("qemu:dirty-bitmap:"), query); return true; } /* * nbd_negotiate_meta_query * * Parse namespace name and call corresponding function to parse body of the * query. * * The only supported namespaces are 'base' and 'qemu'. * * Return -errno on I/O error, 0 if option was completely handled by * sending a reply about inconsistent lengths, or 1 on success. */ static int nbd_negotiate_meta_query(NBDClient *client, NBDExportMetaContexts *meta, Error **errp) { int ret; g_autofree char *query = NULL; uint32_t len; ret = nbd_opt_read(client, &len, sizeof(len), errp); if (ret <= 0) { return ret; } len = cpu_to_be32(len); if (len > NBD_MAX_STRING_SIZE) { trace_nbd_negotiate_meta_query_skip("length too long"); return nbd_opt_skip(client, len, errp); } query = g_malloc(len + 1); ret = nbd_opt_read(client, query, len, errp); if (ret <= 0) { return ret; } query[len] = '\0'; if (strlen(query) != len) { return nbd_opt_invalid(client, errp, "Embedded NUL in query for option %s", nbd_opt_lookup(client->opt)); } if (nbd_meta_base_query(client, meta, query)) { return 1; if (nbd_meta_qemu_query(client, meta, query)) { return 1; } trace_nbd_negotiate_meta_query_skip("unknown namespace"); return 1; } -- Best regards, Vladimir
On 9/26/20 7:58 AM, Vladimir Sementsov-Ogievskiy wrote: > 25.09.2020 23:32, Eric Blake wrote: >> We had a premature optimization of trying to read as little from the >> wire as possible while handling NBD_OPT_SET_META_CONTEXT in phases. >> But in reality, we HAVE to read the entire string from the client >> before we can get to the next command, and it is easier to just read >> it all at once than it is to read it in pieces. And once we do that, >> several functions end up no longer performing I/O, and no longer need >> to return a value. >> >> While simplifying this, take advantage of g_str_has_prefix for less >> repetition of boilerplate string length computation. >> >> Our iotests still pass; I also checked that libnbd's testsuite (which >> covers more corner cases of odd meta context requests) still passes. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> >> -/* Read strlen(@pattern) bytes, and set @match to true if they match >> @pattern. >> - * @match is never set to false. >> - * >> - * Return -errno on I/O error, 0 if option was completely handled by >> - * sending a reply about inconsistent lengths, or 1 on success. >> - * >> - * Note: return code = 1 doesn't mean that we've read exactly @pattern. >> - * It only means that there are no errors. >> + >> +/* >> + * Check @ns with @len bytes, and set @match to true if it matches >> @pattern, >> + * or if @len is 0 and the client is performing _LIST_. @match is >> never set >> + * to false. >> */ >> -static int nbd_meta_pattern(NBDClient *client, const char *pattern, >> bool *match, >> - Error **errp) >> +static void nbd_meta_empty_or_pattern(NBDClient *client, const char >> *pattern, >> + const char *ns, uint32_t len, > > ns changed its meaning, it's not just a namespace, but the whole query. > I think, better to rename it. Sure, that makes sense. > > Also, it's unusual to pass length together with nul-terminated string, > it seems redundant. > And, it's used only to compare with zero, strlen(ns) == 0 or ns[0] == 0 > is not slower. Good point. > > Also, errp is unused argument. And it violate Error API recommendation > to not create void functions with errp. Another simplification. Looks like I'll be spinning v2. > > Also we can use bool return instead of return through pointer. That changes the callers, but it's not necessarily a bad thing; whereas we were doing: if (cond) { nbd_meta_pattern(..., &match, ...); } which either leaves match unchanged or sets it to true, we could instead do: if (nbd_meta_pattern(...)) { match = true; } My only worry is that the more changes I make, the harder the patch is to read. I don't know if it's worth breaking it into steps, or just doing all the simplifications in one blow. >> +static void nbd_meta_base_query(NBDClient *client, >> NBDExportMetaContexts *meta, >> + const char *ns, uint32_t len, Error >> **errp) >> { >> - return nbd_meta_empty_or_pattern(client, "allocation", len, >> - &meta->base_allocation, errp); >> + nbd_meta_empty_or_pattern(client, "allocation", ns + 5, len - 5, > > This one is correct, but would be simpler, if only subquery (after > colon) passed here. > (Really, no reason to pass 'base:' to _base_ func) > > Hmm, I see that it gives a possibility to use > meta->exp->export_bitmap_context directly. Yeah, that's where it got interesting. A direct strcmp() is nice, but if we strip a prefix in one place, we have to strip it in the other. I could go either way. >> + * @len is the length of @ns, including the `qemu:' prefix. >> + */ >> +static void nbd_meta_qemu_query(NBDClient *client, >> NBDExportMetaContexts *meta, >> + const char *ns, uint32_t len, Error >> **errp) >> { >> - bool dirty_bitmap = false; >> - size_t dirty_bitmap_len = strlen("dirty-bitmap:"); >> - int ret; >> - >> if (!meta->exp->export_bitmap) { >> trace_nbd_negotiate_meta_query_skip("no dirty-bitmap >> exported"); >> - return nbd_opt_skip(client, len, errp); >> - } >> - >> - if (len == 0) { >> + } else if (len == 0) { > > s/0/5/ ? Oh, good catch. Using 0 is an unintended logic change, but none of our iotests caught it, so we're also missing test coverage. I'm working on adding 'nbd_opt_list_meta_context()' to libnbd, which will add testsuite coverage of this. >> + } else if (!g_str_has_prefix(ns + 5, "dirty-bitmap:")) { >> trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); >> - return nbd_opt_skip(client, len, errp); >> + } else { >> + trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); >> + nbd_meta_empty_or_pattern(client, >> meta->exp->export_bitmap_context, >> + ns, len, &meta->bitmap, errp); > > hmm. _empty_ is impossible here, as ns includes "qemu:dirty-bitmap".. Ultimately, we want something like: if starts with "base:": if nothing else: if list mode: return "base:allocation" else: return empty list else if "base:allocation": return "base:allocation" else return empty list else if starts with "qemu:": if nothing else: if list mode: return all possible sub-qemu contexts else: return empty list else if starts with "qemu:dirty-bitmap:": if nothing else: if list mode: return all possible dirty-bitmap contexts (right now, just one, but I want to allow an array of bitmaps in the future) else: return empty list else if exact match: return that bitmap else: return empty list else if "qemu:allocation-depth" (from next patch): return "allocation-depth" else: return empty list else: return empty list Maybe some function renames will help. >> if (ret <= 0) { >> return ret; >> } >> + ns[len] = '\0'; >> + if (strlen(ns) != len) { >> + return nbd_opt_invalid(client, errp, >> + "Embedded NUL in query for option %s", >> + nbd_opt_lookup(client->opt)); >> + } > > Hmm, that's a new good check. Intersting, is it specified in NBD > protocol, that > NUL shouldn't be in a string.. Probably it can be a separate patch, with > new nbd_opt_string_read, which will check this thing. But I'm OK with it > as is > in this patch. I'll separate it. > To avoid all this pointer arithmetic, what about something like this (I > didn't check, just an idea): > I'll see what parts of that I can use in v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
diff --git a/nbd/server.c b/nbd/server.c index 982de67816a7..0d2d7e52058f 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2018 Red Hat, Inc. + * Copyright (C) 2016-2020 Red Hat, Inc. * Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws> * * Network Block Device Server Side @@ -792,135 +792,64 @@ static int nbd_negotiate_send_meta_context(NBDClient *client, return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0; } -/* Read strlen(@pattern) bytes, and set @match to true if they match @pattern. - * @match is never set to false. - * - * Return -errno on I/O error, 0 if option was completely handled by - * sending a reply about inconsistent lengths, or 1 on success. - * - * Note: return code = 1 doesn't mean that we've read exactly @pattern. - * It only means that there are no errors. + +/* + * Check @ns with @len bytes, and set @match to true if it matches @pattern, + * or if @len is 0 and the client is performing _LIST_. @match is never set + * to false. */ -static int nbd_meta_pattern(NBDClient *client, const char *pattern, bool *match, - Error **errp) +static void nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern, + const char *ns, uint32_t len, + bool *match, Error **errp) { - int ret; - char *query; - size_t len = strlen(pattern); - - assert(len); - - query = g_malloc(len); - ret = nbd_opt_read(client, query, len, errp); - if (ret <= 0) { - g_free(query); - return ret; - } - - if (strncmp(query, pattern, len) == 0) { + if (len == 0) { + if (client->opt == NBD_OPT_LIST_META_CONTEXT) { + *match = true; + } + trace_nbd_negotiate_meta_query_parse("empty"); + } else if (strcmp(pattern, ns) == 0) { trace_nbd_negotiate_meta_query_parse(pattern); *match = true; } else { trace_nbd_negotiate_meta_query_skip("pattern not matched"); } - g_free(query); - - return 1; -} - -/* - * Read @len bytes, and set @match to true if they match @pattern, or if @len - * is 0 and the client is performing _LIST_. @match is never set to false. - * - * Return -errno on I/O error, 0 if option was completely handled by - * sending a reply about inconsistent lengths, or 1 on success. - * - * Note: return code = 1 doesn't mean that we've read exactly @pattern. - * It only means that there are no errors. - */ -static int nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern, - uint32_t len, bool *match, Error **errp) -{ - if (len == 0) { - if (client->opt == NBD_OPT_LIST_META_CONTEXT) { - *match = true; - } - trace_nbd_negotiate_meta_query_parse("empty"); - return 1; - } - - if (len != strlen(pattern)) { - trace_nbd_negotiate_meta_query_skip("different lengths"); - return nbd_opt_skip(client, len, errp); - } - - return nbd_meta_pattern(client, pattern, match, errp); } /* nbd_meta_base_query * * Handle queries to 'base' namespace. For now, only the base:allocation - * context is available. 'len' is the amount of text remaining to be read from - * the current name, after the 'base:' portion has been stripped. - * - * Return -errno on I/O error, 0 if option was completely handled by - * sending a reply about inconsistent lengths, or 1 on success. + * context is available. @len is the length of @ns, including the 'base:' + * prefix. */ -static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta, - uint32_t len, Error **errp) +static void nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta, + const char *ns, uint32_t len, Error **errp) { - return nbd_meta_empty_or_pattern(client, "allocation", len, - &meta->base_allocation, errp); + nbd_meta_empty_or_pattern(client, "allocation", ns + 5, len - 5, + &meta->base_allocation, errp); } /* nbd_meta_bitmap_query * * Handle query to 'qemu:' namespace. - * @len is the amount of text remaining to be read from the current name, after - * the 'qemu:' portion has been stripped. - * - * Return -errno on I/O error, 0 if option was completely handled by - * sending a reply about inconsistent lengths, or 1 on success. */ -static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, - uint32_t len, Error **errp) + * @len is the length of @ns, including the `qemu:' prefix. + */ +static void nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, + const char *ns, uint32_t len, Error **errp) { - bool dirty_bitmap = false; - size_t dirty_bitmap_len = strlen("dirty-bitmap:"); - int ret; - if (!meta->exp->export_bitmap) { trace_nbd_negotiate_meta_query_skip("no dirty-bitmap exported"); - return nbd_opt_skip(client, len, errp); - } - - if (len == 0) { + } else if (len == 0) { if (client->opt == NBD_OPT_LIST_META_CONTEXT) { meta->bitmap = true; } trace_nbd_negotiate_meta_query_parse("empty"); - return 1; - } - - if (len < dirty_bitmap_len) { - trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); - return nbd_opt_skip(client, len, errp); - } - - len -= dirty_bitmap_len; - ret = nbd_meta_pattern(client, "dirty-bitmap:", &dirty_bitmap, errp); - if (ret <= 0) { - return ret; - } - if (!dirty_bitmap) { + } else if (!g_str_has_prefix(ns + 5, "dirty-bitmap:")) { trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:"); - return nbd_opt_skip(client, len, errp); + } else { + trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); + nbd_meta_empty_or_pattern(client, meta->exp->export_bitmap_context, + ns, len, &meta->bitmap, errp); } - - trace_nbd_negotiate_meta_query_parse("dirty-bitmap:"); - - return nbd_meta_empty_or_pattern( - client, meta->exp->export_bitmap_context + - strlen("qemu:dirty_bitmap:"), len, &meta->bitmap, errp); } /* nbd_negotiate_meta_query @@ -930,22 +859,13 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, * * The only supported namespaces are 'base' and 'qemu'. * - * The function aims not wasting time and memory to read long unknown namespace - * names. - * * Return -errno on I/O error, 0 if option was completely handled by * sending a reply about inconsistent lengths, or 1 on success. */ static int nbd_negotiate_meta_query(NBDClient *client, NBDExportMetaContexts *meta, Error **errp) { - /* - * Both 'qemu' and 'base' namespaces have length = 5 including a - * colon. If another length namespace is later introduced, this - * should certainly be refactored. - */ int ret; - size_t ns_len = 5; - char ns[5]; + g_autofree char *ns = NULL; uint32_t len; ret = nbd_opt_read(client, &len, sizeof(len), errp); @@ -958,27 +878,29 @@ static int nbd_negotiate_meta_query(NBDClient *client, trace_nbd_negotiate_meta_query_skip("length too long"); return nbd_opt_skip(client, len, errp); } - if (len < ns_len) { - trace_nbd_negotiate_meta_query_skip("length too short"); - return nbd_opt_skip(client, len, errp); - } - len -= ns_len; - ret = nbd_opt_read(client, ns, ns_len, errp); + ns = g_malloc(len + 1); + ret = nbd_opt_read(client, ns, len, errp); if (ret <= 0) { return ret; } + ns[len] = '\0'; + if (strlen(ns) != len) { + return nbd_opt_invalid(client, errp, + "Embedded NUL in query for option %s", + nbd_opt_lookup(client->opt)); + } - if (!strncmp(ns, "base:", ns_len)) { + if (g_str_has_prefix(ns, "base:")) { trace_nbd_negotiate_meta_query_parse("base:"); - return nbd_meta_base_query(client, meta, len, errp); - } else if (!strncmp(ns, "qemu:", ns_len)) { + nbd_meta_base_query(client, meta, ns, len, errp); + } else if (g_str_has_prefix(ns, "qemu:")) { trace_nbd_negotiate_meta_query_parse("qemu:"); - return nbd_meta_qemu_query(client, meta, len, errp); + nbd_meta_qemu_query(client, meta, ns, len, errp); + } else { + trace_nbd_negotiate_meta_query_skip("unknown namespace"); } - - trace_nbd_negotiate_meta_query_skip("unknown namespace"); - return nbd_opt_skip(client, len, errp); + return 1; } /* nbd_negotiate_meta_queries
We had a premature optimization of trying to read as little from the wire as possible while handling NBD_OPT_SET_META_CONTEXT in phases. But in reality, we HAVE to read the entire string from the client before we can get to the next command, and it is easier to just read it all at once than it is to read it in pieces. And once we do that, several functions end up no longer performing I/O, and no longer need to return a value. While simplifying this, take advantage of g_str_has_prefix for less repetition of boilerplate string length computation. Our iotests still pass; I also checked that libnbd's testsuite (which covers more corner cases of odd meta context requests) still passes. Signed-off-by: Eric Blake <eblake@redhat.com> --- nbd/server.c | 172 ++++++++++++++------------------------------------- 1 file changed, 47 insertions(+), 125 deletions(-)