Re: [Libguestfs] [libnbd PATCH v3 13/18] api: Reset state on changed nbd_set_export_name()

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&gt; --- 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; }