5:05 p.m.
The documentation for nbd_internal_reset_size_and_flags() claims we
should wipe all knowledge of a previously-negotiated export when
swapping export names. However, we weren't actually doing that, which
could lead to a user calling nbd_opt_info() for one export, then
switching to another export name, then having nbd_get_size() still
report the size of the old export. The next patch will add testsuite
coverage (done separately, to make it easier to prove the test catches
our flaw without this fix by applying patches out of order).
While at it, there is no need to wipe state if we change to the same
name. Note, however, that we do not bother to store the last name
we've exposed to the server; as a result, there are instances where we
clear state but the server does not. But in general, this is not a
problem; it's always okay to be more conservative and not utilize
something the server supports, than to be be overly risky and use the
server on the basis of stale state. Here's an example (unlikely to
happen in real code) where we are over-eager in wiping state, even
though the server never knows we considered a different name:
nbd_set_export_name(nbd, "a");
nbd_opt_info(nbd, callback); => NBD_OPT_SET_META_CONTEXT("a")
=> NBD_OPT_INFO("a")
nbd_set_export_name(nbd, "b");
nbd_set_request_meta_context(nbd, false);
nbd_set_export_name(nbd, "a");
nbd_opt_go(nbd); => NBD_OPT_GO("a")
At any rate, this patch, plus several earlier ones, give us the
following state transitions:
export information (nbd_get_size, nbd_is_read_only, nbd_can_trim, ...)
- gated by h->eflags being non-zero, but tracked in multiple
variables such as h->eflags, h->exportsize, h->block_minimum, ...
- made valid by successful NBD_OPT_INFO or NBD_OPT_GO
- wiped by:
- failed NBD_OPT_INFO or NBD_OPT_GO
- any NBD_OPT_STARTTLS (a later patch may add nbd_opt_starttls;
for now, it is not possible to tickle this)
- nbd_set_export_name() changing names (not directly caused by
server state, but because of how many of our interfaces
implicitly reuse h->export_name)
- persists over:
- NBD_OPT_LIST_META_CONTEXT
- NBD_OPT_SET_META_CONTEXT
context mappings (nbd_can_meta_context)
- gated by h->meta_valid, tracked by h->meta_contexts
- made valid (returns 0/1 for all names) by:
- successful NBD_OPT_SET_META_CONTEXT
- if nbd_set_request_meta_context() is true, a successful
nbd_opt_info() or nbd_opt_go() that was unable to set contexts
(server was oldstyle, or structured replies are lacking, or
client didn't use nbd_add_meta_context)
- wiped (returns -1 for all names) by:
- failed NBD_OPT_SET_META_CONTEXT
- any NBD_OPT_STARTTLS
- nbd_set_export_name() changing names
- persists over:
- NBD_OPT_GO, NBD_OPT_LIST
- NBD_OPT_LIST_META_CONTEXT
Fixes: 437a472d ("api: Add nbd_opt_go and nbd_aio_opt_go", v1.3.12)
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
---
lib/handle.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/handle.c b/lib/handle.c
index e59e6160..2be1a0e6 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -232,6 +232,9 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const char
*export_name)
return -1;
}
+ if (strcmp (export_name, h->export_name) == 0)
+ return 0;
+
new_name = strdup (export_name);
if (!new_name) {
set_error (errno, "strdup");
@@ -240,6 +243,7 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const char
*export_name)
free (h->export_name);
h->export_name = new_name;
+ nbd_internal_reset_size_and_flags (h);
h->meta_valid = false;
return 0;
}
--
2.37.3