On Thu, Sep 01, 2022 at 11:21:38AM +0200, Laszlo Ersek wrote:
> On 08/31/22 16:39, Eric Blake wrote:
> > Upstream NBD clarified (see NBD commit 13a4e33a8) that since
> > NBD_OPT_LIST_META_CONTEXT is stateless on the server side, it is
> > acceptable (but not mandatory) for servers to accept it without the
> > client having pre-negotiated structured replies. We aren't quite
> > stateless on the client side yet - that will be fixed in a later patch
> > to keep this one small and easier to test. The testsuite changes pass
> > when using modern nbdkit; however, it skips[*] if we talk to an older
> > server. But since the test also skips with our pre-patch behavior,
> > it's not worth separating it into a separate patch.
> >
> > For more history, qemu-nbd prior to commit da24597d [v6.2] and nbdkit
> > prior to commit c39cba73 [v1.27.3, also backported to 1.26.6] are
> > servers that chose to reject NBD_OPT_LIST_META_CONTEXT without a prior
> > NBD_OPT_STRUCTURED_REPLY.
> >
> > [*]It's easier to skip on server failure than to try and write an
> > nbdkit patch to add yet another --config feature probe just to depend
> > on new-enough nbdkit to gracefully probe in advance if the server
> > should succeed.
> > ---
> > generator/states-newstyle-opt-meta-context.c | 1 -
> > lib/opt.c | 6 +----
> > tests/opt-list-meta.c | 28 +++++++++++++++++---
> > 3 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/generator/states-newstyle-opt-meta-context.c
> > b/generator/states-newstyle-opt-meta-context.c
> > index a6a5271..5c65454 100644
> > --- a/generator/states-newstyle-opt-meta-context.c
> > +++ b/generator/states-newstyle-opt-meta-context.c
> > @@ -34,7 +34,6 @@ STATE_MACHINE {
> > meta_vector_reset (&h->meta_contexts);
> > if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> > assert (h->opt_mode);
> > - assert (h->structured_replies);
> > assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> > opt = h->opt_current;
> > }
>
> Can we introduce some xfuncname pattern for these "state machine" C
> files? The STATE_MACHINE hunk header is totally useless. I'd like
> "NEWSTYLE.OPT_META_CONTEXT.START". :)
Yes, that's a side ask, but I would love it too. Git has a default
xfuncname pattern for .c files, so it will be interesting to see if I
can figure out .gitattributes that would let us attach additional
patterns on to the generator/*.c files without penalizing normal .c
files.
>
> Regarding the code -- with the removal of the assertion from the LIST
> branch, but preserving a similar check (albeit not an assert()) on the
> SET branch, the comment covering *both* branches is now out of date:
>
> /* If the server doesn't support SRs then we must skip this group.
Indeed it is. I already posted a followup patch to 11/12 where I
improved the comment to its final state; I will grab the appropriate
parts of that comment and revise it through the series to match
changes as they come in.
For 2/12, that results in:
diff --git c/generator/states-newstyle-opt-meta-context.c
w/generator/states-newstyle-opt-meta-context.c
index a6a5271a..dd5a6ded 100644
--- c/generator/states-newstyle-opt-meta-context.c
+++ w/generator/states-newstyle-opt-meta-context.c
@@ -23,9 +23,27 @@ STATE_MACHINE {
size_t i;
uint32_t len, opt;
- /* If the server doesn't support SRs then we must skip this group.
- * Also we skip the group if the client didn't request any metadata
- * contexts, when doing SET (but an empty LIST is okay).
+ /* This state group is reached from:
+ * h->opt_mode == false (h->current_opt == 0):
+ * nbd_connect_*()
+ * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO
+ * h->opt_mode == true (h->current_opt matches calling API):
+ * nbd_opt_info()
+ * -> conditionally use SET, next state OPT_GO for NBD_OPT_INFO
+ * nbd_opt_go()
+ * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO
+ * nbd_opt_list_meta_context()
+ * -> conditionally use LIST, next state NEGOTIATING
+ *
+ * For now, we start by unconditionally clearing h->exportsize and friends,
+ * as well as h->meta_contexts and h->meta_valid.
+ * SET then manipulates h->meta_contexts, and sets h->meta_valid on success.
+ * If SET is conditional, we skip it if structured replies were
+ * not negotiated, or if there were no contexts to request.
+ * If OPT_GO is later successful, it populates h->exportsize and friends,
+ * and also sets h->meta_valid if we skipped SET here.
+ * LIST is conditional, skipped if structured replies were not negotiated.
+ * There is a callback if and only if the command is LIST.
*/
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
nbd_internal_reset_size_and_flags (h);
For this patch, it changes as:
diff --git c/generator/states-newstyle-opt-meta-context.c
w/generator/states-newstyle-opt-meta-context.c
index c45ff129..020a7adf 100644
--- c/generator/states-newstyle-opt-meta-context.c
+++ w/generator/states-newstyle-opt-meta-context.c
@@ -33,7 +33,7 @@ STATE_MACHINE {
* nbd_opt_go()
* -> conditionally use SET, next state OPT_GO for NBD_OPT_GO
* nbd_opt_list_meta_context()
- * -> conditionally use LIST, next state NEGOTIATING
+ * -> unconditionally use LIST, next state NEGOTIATING
*
* For now, we start by unconditionally clearing h->exportsize and friends,
* as well as h->meta_contexts and h->meta_valid.
@@ -42,8 +42,7 @@ STATE_MACHINE {
* not negotiated, or if there were no contexts to request.
* If OPT_GO is later successful, it populates h->exportsize and friends,
* and also sets h->meta_valid if we skipped SET here.
- * LIST is conditional, skipped if structured replies were not negotiated.
- * There is a callback if and only if the command is LIST.
+ * There is a callback if and only if the command is unconditional.
*/
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
nbd_internal_reset_size_and_flags (h);
In 4/12, it is further modified:
diff --git c/generator/states-newstyle-opt-meta-context.c
w/generator/states-newstyle-opt-meta-context.c
index 37022594..28100199 100644
--- c/generator/states-newstyle-opt-meta-context.c
+++ w/generator/states-newstyle-opt-meta-context.c
@@ -35,13 +35,12 @@ STATE_MACHINE {
* nbd_opt_list_meta_context()
* -> unconditionally use LIST, next state NEGOTIATING
*
- * For now, we start by unconditionally clearing h->exportsize and friends,
- * as well as h->meta_contexts and h->meta_valid.
- * SET then manipulates h->meta_contexts, and sets h->meta_valid on success.
- * If SET is conditional, we skip it if structured replies were
- * not negotiated, or if there were no contexts to request.
+ * For now, we start by unconditionally clearing h->exportsize and friends.
+ * If SET is conditional, we skip it if h->request_meta is false, if
+ * structured replies were not negotiated, or if no contexts to request.
* If OPT_GO is later successful, it populates h->exportsize and friends,
- * and also sets h->meta_valid if we skipped SET here.
+ * and also sets h->meta_valid if h->request_meta but we skipped SET here.
+ * SET then manipulates h->meta_contexts, and sets h->meta_valid on success.
* There is a callback if and only if the command is unconditional.
*/
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
And having typed that up, I'm now starting to think I should tweak
patch 7/12 to move the call to nbd_internal_reset_size_and_flags() out
of NEWSTYLE.OPT_META_CONTEXT.START and into NEWSTYLE.OPT_GO.START, to
further solidify that it is the act of NBD_OPT_INFO/GO that wipes
h->exportsize, not the act of NBD_OPT_SET/LIST_META_CONTEXT.
> > +++ b/tests/opt-list-meta.c
...
> >
> > + nbd_opt_abort (nbd);
> > + nbd_close (nbd);
> > +
> > exit (EXIT_SUCCESS);
> > }
> >
>
> On a tangent: why do we have nbd_opt_abort() separately from
> nbd_close()? What further steps would be valid between the two?
nbd_opt_abort() is optional; it says to inform the server of our
intent to do a clean disconnect during negotiation. If you skip it and
just do nbd_close(), the server is more likely to see an unexpected
EOF and warn that we went away unexpectedly. It is a direct
counterpart to nbd_shutdown(0) informing the server that we want a
clean disconnect during transmission.
As for what we can do between the two - nbd_opt_abort() generally
moves the handle to state CLOSED, but there you can still do things
like nbd_get_size() to see what had happened while the handle was
alive; while nbd_close() wipes the handle altogether.
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Should I apply your R-b with my incremental doc tweaks incorporated
along the way, or are you going to want to see a v3 respin with
everything in one place (especially now that I'm thinking of further
tweaks in 7/12)?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
