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

On Wed, Aug 24, 2022 at 01:56:03PM +0200, Laszlo Ersek wrote: Yep, exactly what happened; where the subsequent patch is still not quite polished enough to push to the list (but hopefully later today). Will fix before committing. Right now, the code base does: nbd_set_export_name() => update h->export_name; no server interaction nbd_add_meta_context() => append to h->request_meta_contexts; no server interaction nbd_clear_meta_contexts() => wipe all of h->request_meta_contexts; no server interaction nbd_opt_list_meta_context() => Issue NBD_OPT_LIST_META_CONTEXT to the server using h->request_meta_contexts as its query list and h->export_name, but collect the results only in the user's callback - not stateful on the server, and only stateful in the client if the callback does something to preserve the server's answers nbd_opt_info()/nbd_opt_go() => Issue NBD_OPT_SET_META_CONTEXT using h->request_meta_contexts as its query list and h->export_name, collect the results into h->meta_contexts, then issue NBD_OPT_INFO/GO to the server. Stateful, because the server's replies map the users context names (strings) to the servers ids (uint32_t); and later NBD_CMD_BLOCK_STATUS refers only to the server's 32-bit id, even though the libnbd API does not directly expose those ids. nbd_can_meta_context() => Query whether h->meta_contexts contains a server reply for an earlier call that triggered NBD_OPT_SET_META_CONTEXT and which has not been wiped by changing state via nbd_internal_reset_size_and_flags(); no server interaction. with the additional caveat that all the nbd_opt_* functions are unreachable unless you do nbd_set_opt_mode(true) to enable manual option negotiation (the default is that nbd_connect_*() behaves as if it does nbd_opt_go() on your behalf). The promised upcoming patch will add: nbd_set_request_meta_context(false) => Skip the NBD_OPT_SET_META_CONTEXT portion in nbd_opt_info()/nbd_opt_go() nbd_opt_set_meta_context() => Issue NBD_OPT_SET_META_CONTEXT to the server using h->request_meta_contexts as its query list and h->export_name, and collect the results into both the user's callback and h->meta_contexts at the same time. Stateful - wipes out any ids returned by an earlier SET_META_CONTEXT, but the ids returned by the server can survive a number of other NBD_OPT_ commands prior to NBD_OPT_GO, provided that NBD_OPT_GO uses the same export name as NBD_OPT_SET_META_CONTEXT. In that manner, it will become possible for manual option haggling to set the meta contexts at a different point in the negotiation phase, useful when trying to trigger various server corner cases. The server wipes meta context state any time it gets NBD_OPT_SET_META_CONTEXT or NBD_OPT_GO with a different export name than before. But since we don't pass in export name as a direct parameter to either nbd_opt_go() or the upcoming nbd_opt_set_meta_context(), we instead need to wipe the state when we change h->export_name (that is, at nbd_set_export_name()). This does mean that if a client calls nbd_set_export_name(nbd, "a"), nbd_set_meta_context(...), nbd_set_export_name(nbd, "b"), nbd_set_export_name(nbd, "a"), nbd_opt_go(nbd), that the server will not observe the name change, and will actually support the previously-negotiated contexts, but libnbd will have wiped access to those, and trigger client-side failure on attempts to call nbd_block_status() on the grounds that it doesn't remember any negotiated contexts. But a client actually doing this is unlikely. Typo on my part. The API nbd_block_status() issues NBD_CMD_BLOCK_STATUS to the server. Here, it is intended. The NBD spec says a client should not issue NBD_OPT_BLOCK_STATUS to the server unless it successfully negotiated NBD_OPT_SET_META_CONTEXT with the same export name later passed to NBD_OPT_GO, without intervening commands that might wipe that state. The client code in nbd_block_status() tries to enforce that it won't confuse the server by checking whether h->meta_contexts is non-NULL before it will issue NBD_CMD_BLOCK_STATUS. But once we allow manual control over setting meta contexts, rather than tightly coupling it with NBD_OPT_GO, we need to make sure we wipe h->meta_contexts in at least all places the server wipes state (even if we also happen to wipe it in more places, as in my "a"/set/"b"/"a"/go example). Maybe I can separate it; the test would be calling nbd_opt_info() to set h->meta_contexts, nbd_can_meta_context() to verify that "base:allocation" is supported by the server, nbd_opt_list_meta_context() to do a query, then nbd_can_meta_context() again to see whether "base:allocation" is still remembered or was accidentally wiped. Fair enough. Basically, nbd_opt_list_meta_context() hands the server's answers to the user's callback instead of by modifications to h->meta_contexts, so it does not impact future nbd_can_meta_context(). I'll see if I can reword that a bit. This hunk fixes the nbd_opt_list_meta_context() accidentally wiping context when it doesn't have to, and this hunk fixes the missing wipe (the testsuite coverage so far only covered this case). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org