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

On 08/24/22 23:20, Eric Blake wrote: Thanks for writing this up; it's a lot to digest. I think I understand it now. There's a whole lot of state here, which is something I don't really like. (I don't know the design background however.) FWIW I've found this stuff easier to understand by thinking about it in terms of requests/responses, and by considering the client-side state only a "lightweight state", solely existing for implementing the request/response patterns. In other words, I kind of consider the client-side state "ad hoc" (whereas the server-side state is heavy-weight state). The overlap between nbd_opt_list_meta_context() and nbd_opt_info() is especially confusing. For the following reasons: - both functions perform the same listing at the base level (build an intersection between the client's desired meta context list and the server's supported meta context list) - the intersection is consumed differently on the client (the first function reports results through a callback, the second one requires subsequent, repeated calls to nbd_can_meta_context()). - the first function is stateless (considering only the server-side state "state" here), the second is stateful. While this is well reflected by the opcode names LIST vs. SET, those opcodes are hidden from the function names. In particular, the second function says "info" in the name, which I would not expect to change state on the server. - NBD_OPT_INFO in particular looks like a useless opcode. NBD_OPT_SET_META_CONTEXT handles the filtering (intersection) and mapping to numeric IDs. NBD_OPT_GO transitions the server to the "ready" state (completes negotiation). So those two look useful. I don't understand what the precise responsibility is that *only* NBD_OPT_INFO can cover. The manual at <https://libguestfs.org/nbd_opt_info.3.html> says that nbd_opt_info() enables export size querying (for example), but that kind of querying is just as possible without nbd_opt_info(); only nbd_set_opt_mode() is needed -- see the example code at <https://libguestfs.org/nbd_set_opt_mode.3.html>;. This makes sense for nbd_opt_go(), as it will then retain a single responsibility: transitioning the server to the ready state. However, with SET_META_CONTEXT skipped, nbd_opt_go() appears to be reduced to sending the NBD_OPT_INFO opcode -- and that one looks entirely useless to me (see above). Right, so nbd_opt_set_meta_context() is the "first half" extracted from the current behavior of nbd_opt_info()/nbd_opt_go(). (Again with my personal confusion that the "second half" of nbd_opt_info(), i.e., the sending of the NB_OPT_INFO opcode, looks useless.) So this is the problem with stateful protocols; we need to remember and recognize the points at which the state goes stale and must be cleared. I wonder why the NBD protocol was not designed to take as many parameters as possible per-request (minimizing the state in both client and server), and only turn the kind of information into actual "state" that is required by the *bulk* operations (basically the block-level operations on the device, such as read, write, zero/trim, etc) where the parameter repetitions would have significant bandwidth and computation (formatting/parsing) cost. The cost of the negotiation phase looks trivial in comparison to the cost of the "ready state" work (IIRC the "ready state"); all the cached information does not seem to buy us much. OK. The wording of your commit message ("actively dangerous") confused me, as I didn't expect the client to be able to break the server (for any definition of "break"). However, if the spec uses the "SHOULD NOT" term, then preventing that sequence of operations in the library makes sense. .... Again, anything around nbd_opt_info() keeps confusing me. What we actually need here is the NBD_OPT_SET_META_CONTEXT. Currently, we can do that in two ways, with nbd_opt_info() and nbd_opt_go(). The latter is no good, as it kicks us out of the negotiation phase. The former I *dislike*, because it does something *in addition* to NBD_OPT_SET_META_CONTEXT that I don't understand -- namely the NBD_OPT_INFO opcode. Therefore, the new function nbd_opt_set_meta_context() would be *just right* for this step, as it exposes NBD_OPT_SET_META_CONTEXT (and the population of h->meta_contexts) in isilation. Perhaps introduce the new function before updating the test case? Upon re-reading the commit message, after your explanation, I think the "but" is fine, but benefits from a bit of expanding: Conversely, calling nbd_opt_list_meta_context() does not need to wipe the list - it does not touch h->meta_contexts, but relies on the user's callback instead, *for collecting the list of those metacontexts that the server acknowledges*. How does it sound? Laszlo