Re: [Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR

On Thu, Sep 01, 2022 at 11:21:38AM +0200, Laszlo Ersek wrote: 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. 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. ... 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. 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