On 09/01/22 17:08, Eric Blake wrote:
> On Wed, Aug 31, 2022 at 09:39:23AM -0500, Eric Blake wrote:
>> Since NBD_OPT_LIST_META_CONTEXTS is stateless from the server's point
>> of view, there is no reason to make it clear state on the client's
>> side. Since we do not otherwise modify h->meta_contexts while
>> processing nbd_opt_list_meta_contexts(), we also should not wipe it;
>> similarly, while clearing h->exportsize makes sense if we know we are
>> going to attempt NBD_OPT_INFO or NBD_OPT_GO, it should not be
>> attempted merely because we are doing NBD_OPT_LIST_META_CONTEXTS.
>>
>> Testsuite coverage for this is in the next patch, for ease of review
>> (that is, this is one case where it is easy to swap the patch order to
>> see a failure fixed by this patch).
>> ---
>> generator/states-newstyle-opt-meta-context.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Given the review comments on 3/12, I'm thinking of squashing in:
>
> diff --git c/generator/states-newstyle-opt-go.c
> w/generator/states-newstyle-opt-go.c
> index 1ca5f099..9b05bec1 100644
> --- c/generator/states-newstyle-opt-go.c
> +++ w/generator/states-newstyle-opt-go.c
> @@ -22,6 +22,7 @@ STATE_MACHINE {
> NEWSTYLE.OPT_GO.START:
> uint16_t nrinfos = 0;
>
> + nbd_internal_reset_size_and_flags (h);
> if (h->request_block_size)
> nrinfos++;
> if (h->full_info)
> diff --git c/generator/states-newstyle-opt-meta-context.c
> w/generator/states-newstyle-opt-meta-context.c
> index 3e66b106..6149d61d 100644
> --- c/generator/states-newstyle-opt-meta-context.c
> +++ w/generator/states-newstyle-opt-meta-context.c
> @@ -35,12 +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.
> * 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 h->request_meta but we skipped SET here.
> - * SET then manipulates h->meta_contexts, and sets h->meta_valid on
> success.
> + * SET then manipulates h->meta_contexts, and sets h->meta_valid on
> + * success, while LIST is stateless.
> * There is a callback if and only if the command is unconditional.
> */
> assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
> @@ -52,7 +52,6 @@ STATE_MACHINE {
> else {
> assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> opt = NBD_OPT_SET_META_CONTEXT;
> - nbd_internal_reset_size_and_flags (h);
> if (h->request_meta) {
> for (i = 0; i < h->meta_contexts.len; ++i)
> free (h->meta_contexts.ptr[i].name);
>
>
I think you may have queued so many changes for v3 around this part of
the code that I feel like skipping this patch for now (in v2).
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
