[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] nbd: Simplify meta-context parsing
From: |
Eric Blake |
Subject: |
Re: [PATCH 1/3] nbd: Simplify meta-context parsing |
Date: |
Mon, 28 Sep 2020 09:05:54 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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