Re: [Libguestfs] [libnbd PATCH v2 04/12] api: Add nbd_set_request_meta_context()

On Mon, Sep 05, 2022 at 04:35:28PM +0200, Laszlo Ersek wrote: ... Good question. Thanks to tweaks I made to 2/12 before pushing that one (55b09667), in v3 this patch 4/12 will be starting with a better comment pre-patch: /* This state group is reached from: * h->opt_mode == false (h->opt_current == 0): * nbd_connect_*() * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO * h->opt_mode == true (h->opt_current 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. * If SET is conditional, we skip it if structured replies were * not negotiated, or if there were no contexts to request. * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. * 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. */ And this is the key point - here, we are slamming opt to SET_META_CONTEXT, while leaving h->opt_current as 0 (nbd_connect_*), GO, or INFO. h->opt_current won't be SET_META_CONTEXT until later in the series when the nbd_opt_set_meta_context() API is added; at which point, the v3 patch will update the comment to match. That's where the confusion stemmed from; without the comment explaining the 4 possible values of h->opt_current on entry (5 after nbd_opt_set_meta_context() is introduced later), it's harder to see why it made more sense to isolate the decision to short-circuit over to OPT_GO (what the comment calls "conditional SET") separately from the decision of whether to reset h->meta_contexts (we want the reset for SET but not for LIST, even if we don't end up sending SET over the wire). For this patch, the following hunk would indeed be less convoluted, but later in the series when nbd_opt_set_meta_context() is added, I really did need to rearrange the control flow; whether doing it in this patch makes the most sense, or deferring the control flow rearrangement to later would have been wiser, I don't know, but we'll see if the comments in v3 make it easier to follow. As stated above, in this patch, we can only enter with h->opt_current == 0, LIST, GO, or INFO. And for what it's worth, in an earlier iteration of my patch, my logic did look more like your proposed alternative hunk. Hmm, good question. It is undefined to call nbd_FOO(nbd) if you have previously called nbd_close(nbd); but to minimize the risk of nbd_can_meta_context() doing something weird if the UAF hasn't already hit the worse case of something else clobbering the memory, then yes, explicitly clearing h->meta_valid during close is probably wise. I'll add that into v3. Yep, and I had already mentioned it in my reply to 0/12; but by failing to also mention it in reply to this message, I've cost you some review time; sorry about that. At any rate, I've fixed that already in my tree for v3 to quit adding libtool-generated files. I don't know if it would have been any easier had it been an actual binary instead of a libtool script (would git have given me the one-line 'binary diff' message, or blown up the email into trying to represent the binary?) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org