Re: [Libguestfs] [libnbd PATCH 1/2] api: Better use of nbd_internal_reset_size_and_flags

The documentation for nbd_internal_reset_size_and_flags() claims we should wipe all knowledge of previously-negotiated meta contexts 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. At present, this is only mildly confusing, but once the next patch exposes nbd_opt_set_meta_context() for user control,

it would become actively dangerous: negotiating meta contexts for one export, then switching to a different export name before nbd_opt_go, can lead us into calling nbd_cmd_block_status() when the server is not expecting it.

While fixing this, code things to check whether we are actually swapping export names; an idempotent nbd_set_export_name() is not observable by the server, and therefore does not need to invalidate what has already been negotiated.

Conversely, calling nbd_opt_list_meta_context() does not need to wipe the list - it does not touch h->meta_contexts, but relies on the user's callback instead.

--- generator/states-newstyle-opt-meta-context.c | 7 +++---- lib/handle.c | 4 ++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 30b9617..76f1032 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -28,7 +28,6 @@ STATE_MACHINE { * contexts, when doing SET (but an empty LIST is okay). */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); - nbd_internal_reset_size_and_flags (h); if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) { assert (h->opt_mode); assert (h->structured_replies); @@ -37,6 +36,8 @@ STATE_MACHINE { } else { assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); + nbd_internal_reset_size_and_flags (h); + assert (h->meta_contexts == NULL); opt = NBD_OPT_SET_META_CONTEXT; if (!h->structured_replies || h->request_meta_contexts.len == 0) { SET_NEXT_STATE (%^OPT_GO.START); @@ -44,8 +45,6 @@ STATE_MACHINE { } } - assert (h->meta_contexts == NULL); - /* Calculate the length of the option request data. */ len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */; for (i = 0; i < h->request_meta_contexts.len; ++i) diff --git a/lib/handle.c b/lib/handle.c index 8713e18..2aa89b9 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -219,6 +219,10 @@ 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; + + nbd_internal_reset_size_and_flags (h); new_name = strdup (export_name); if (!new_name) { set_error (errno, "strdup");