On Thu, Aug 25, 2022 at 12:13:42PM +0200, Laszlo Ersek wrote: [I wrote my reply out-of-order in various spurts, apologies if I repeat myself, or if I make an argument earlier in the mail that doesn't make sense until you read more background later in the mail] ... The design background does play a big role. When we first started libnbd, we did not have an easy way to write an API that would take a string vector as a single argument that the generator could then translate nicely into C, OCaml, and Python APIs (this was before we later added golang translations). So instead of having a single API: nbd_connect_with_contexts(uri, vector_of_contexts) it was thus easier given the limitation of our generator at the time to have: nbd_add_meta_context(a) [appends to h->request_meta_contexts] nbd_add_meta_context(b) [appends to h->request_meta_contexts] nbd_connect(uri) [implicitly uses h->request_meta_contexts from earlier adds] And we definitely don't have a clean way for a single API to produce a direct output of a string vector (easy in Python, but hard in C); our approach there was initially using client-side state (build the list internally to the nbd_handle, let the user query it after the fact: nbd_connect_* populates h->meta_contexts followed by nbd_can_meta_context() querying it), although it has also proven viable to utilize a callback function (let the user build their own vector as we pass in one string at a time: nbd_opt_list() nbd_opt_list_meta_context() both use the same callback signature). Also, when we first started, libnbd did everything in nbd_connect_* from opening the socket to NBD_OPT_GO; you could only fine-tune the handshake by what was set prior to nbd_connect_. In libnbd 1.4, we added nbd_set_opt_mode and nbd_opt_* to try and make it more fine-tuned, giving more control to a power user over aspects of handshaking (most obvious is nbdinfo that wants to run NBD_OPT_LIST to learn the list of available exports, then NBD_OPT_INFO and NBD_OPT_SET/LIST_META_CONTEXT on each of those exports in turn), while still keeping the common case simple (if you already know your connection name, nbd_add_meta_context("base:allocation") + nbd_connect_uri(uri) going all the way to NBD_OPT_GO really is nicer than forcing you to spell out all the individual steps). So backwards-compatibility is an issue - the older an API is, the more steps it tends to do by default; we have been introducing new API to break it into smaller independent chunks where that is useful, but still have to keep the old complexity in place to not break existing code bases. So this is client-side state in the reverse direction - now we have lots of APIs for each knob we have added over the years to break large APIs into smaller tasks (see nbd_set_request_structured_replies, nbd_get_request_structured_replies, as well as my proposal to add nbd_set_request_meta_context, ...), which have nothing to do with server state but do impact how other nbd_* APIs will behave. Over time, our generator has gotten better (we've added support for more parameter types), so it may be worth adding API(s) that take the context name list as explicit parameters rather than reusing the implicit list registered one-at-a-time. Where we currently have: nbd_add_meta_context(a) nbd_add_meta_context(b) nbd_opt_list_meta_context(export, callback) it may indeed be worth adding: nbd_opt_list_meta_context_from_list(export, vector, callback) that uses an explicit vector of queries rather than the implied vector previously set by nbd_add_meta_context(). At the same time, our set of nbd_connect_* calls has grown, so adding yet another variant to each of those that takes a context vector may be easy in the generator, but scale exponentially in the number of APIs added and needing unit tests. You are correct that h->request_meta_contexts is light-weight client state (our hack to work around the lack of a single vector argument, when passing a query to NBD_OPT_LIST_META_CONTEXT or NBD_OPT_SET_META_CONTEXT), while h->meta_contexts is more heavy-weight (both the client and the server must agree on the same name-to-id mapping of which contexts were set by the last successful NBD_OPT_SET_META_CONTEXT and not wiped out by other state-clearing commands like NBD_OPT_STARTTLS or NBD_OPT_GO with a different export name; because that name-to-id mapping is essential to later NBD_CMD_BLOCK_STATUS). Thus, we have several conflated issues. - Can we reduce ad hoc client state? Yes, if we extend the generator and add new APIs that take the query vector as a direct argument instead of the indirect h->request_meta_contexts, and/or provide a callback argument to let the caller build up their own reply vector instead of having to query an internal result vector after the fact (so far, not something I have tried, but now you have me curious on whether it is worth it). - Can we get more fine-grained control over negotiation? Yes, nbd_opt_info() and nbd_opt_go() currently default to a sequence of 2 server commands (NBD_OPT_SET_META_CONTEXT, NBD_OPT_INFO/GO), but v2 of this series will add nbd_set_request_meta_context() which cuts out the first half, as well as nbd_opt_set_meta_context() which runs the first half in isolation. - Are we properly tracking when server state changes? No - in some cases we weren't clearing state even when the server does (teach nbd_set_export_name() to clear state); in other cases we clear state even though we don't have to (teach nbd_opt_list_meta_context() to not clear state). v2 is taking me more time to polish than I thought, because I've been trying to break it down into even more individual fixes with accompanying tests, as well as finish the API additions, but I'm liking the direction your review has set me on. In particular, I've noticed that there are really two different groups of client states to track after server interaction: export information (nbd_get_size, nbd_is_read_only, nbd_can_trim, ...) - gated by h->eflags being non-zero, but tracked in multiple variables such as h->eflags, h->exportsize, h->block_minimum, ... - should be made valid by successful NBD_OPT_INFO or NBD_OPT_GO - should be wiped by: - failed NBD_OPT_INFO or NBD_OPT_GO - any NBD_OPT_STARTTLS (but I have to add nbd_opt_starttls to allow that much fine-grained control; on my list of things to add) - nbd_set_export_name() changing names (not directly caused by server state, but because of how many of our interfaces implicitly reuse h->export_name, and where the server would return different state for NBD_OPT_INFO on a different name; currently broken, v1 tried to fix it) - should persist over: - NBD_OPT_LIST_META_CONTEXT (this is stateless in the server, so nbd_opt_list_meta_context() should be stateless in the client; currently broken, v1 tried to fix it) - NBD_OPT_SET_META_CONTEXT (setting contexts does not change the export size, so my new nbd_opt_set_meta_context() should not wipe it) context mappings (nbd_can_meta_context) - currently gated by h->eflags being non-zero, but tracked by h->meta_contexts (v2 will change the gating) - should be made valid (returns 0/1 for all names, according to h->meta_contexts lookup) by: - successful NBD_OPT_SET_META_CONTEXT - if nbd_set_request_meta_context() is true (default once the API is added), a successful nbd_opt_info() or nbd_opt_go() that was unable to set contexts (server was oldstyle, or structured replies are lacking, or client didn't register any names with nbd_add_meta_context; so h->meta_contexts is empty) - should be wiped (returns -1 for all names) by: - failed NBD_OPT_SET_META_CONTEXT (regardless if triggered by nbd_opt_go() or by my new nbd_opt_set_meta_context(), currently broken and unaddressed in v1, see also [1] below) - any NBD_OPT_STARTTLS - nbd_set_export_name() changing names (same story as export name change wiping export information: currently broken, v1 tried to fix it) - should persist over: - bare NBD_OPT_GO, NBD_OPT_LIST (untestable until adding nbd_set_request_meta_context(false) to teach nbd_opt_info() and nbd_opt_go() to skip their first half) - NBD_OPT_LIST_META_CONTEXT (currently broken, v1 tried to fix it) nbd_can_meta_context() is probing the name-to-id table returned by NBD_OPT_SET_META_CONTEXT; but NBD_OPT_LIST_META_CONTEXT does not provide a name-to-id table (the NBD spec says that LIST should use an id of 0 for everything, partly because only the LIST form supports wildcards in both client and server directions; note that a wildcard has no valid id, but 0 can be a valid id under SET). Maybe this will help. NBD_OPT_LIST_META_CONTEXT says "what context names does the server support, where the server can disregard the export name if that makes sense, and where both directions can use wildcards to allow the server to advertise a family of export names and the client to ask for more details within that family". With qemu-nbd as server, LIST_META_CONTEXT([], "") (the empty vector) might currently cause the server to reply with ["base:allocation", "qemu:dirty-bitmap:a", "qemu:dirty-bitmap:b"] (all the contexts it specifically supports on the export ""), while LIST_META_CONTEXT(["qemu:"], "") would only reply with ["qemu:dirty-bitmap:a", "qemu:dirty-bitmap:b"]. But it would also have been possible for qemu-nbd to reply to the empty vector query with ["base:allocation", "qemu:"] ("base:allocation" is supported on all exports, regardless of name, but instead of spelling out individual "qemu:..." contexts available for the specific context, returning the wildcard "qemu:" means that the client must do a further LIST_META_CONTEXT to learn about specific contexts for a given export). Conversely, NBD_OPT_SET_META_CONTEXT does not use wildcards, in either the client or the server direction. It is documented as strictly "the client wants these contexts for this export; the server must supply a context-to-id mapping for the ones it supports, and that mapping is then valid until the next NBD_OPT_SET_META_CONTEXT, NBD_OPT_STARTTLS, or use of NBD_OPT_GO with a different export name". The nbd_opt_list() function is trying to learn as much information about a specific export: what it its size and readonly status (from NBD_OPT_LIST), and what contexts can it support (here, NBD_OPT_SET_META_CONTEXT guarantees correct answers, but is rather heavy-handed in setting server state; NBD_OPT_LIST_META_CONTEXT might return wildcards instead of exact answers, but for nbd_opt_info(), that's probably good enough). Then again, since I wrote it without a callback function, the only way to query which contexts is via nbd_can_meta_context(), which requires the name-to-id mapping, which we get from NBD_OPT_SET_META_CONTEXT. Of course, once I add nbd_set_request_meta_context(), we can skip the NBD_OPT_SET_META_CONTEXT done by nbd_opt_info(), and let the user manually use nbd_opt_list_meta_context() for its own stateless NBD_OPT_LIST_META_CONTEXT query of the supported contexts. So it may also be worth a new API: nbd_opt_info_with_contexts(export_name, vector_of_queries, callback) to be used in place of nbd_add_meta_context(context) nbd_opt_info(export_name) nbd_can_meta_context(context) for less stateful pollution. Useless when you already know what export you want to connect to. But useful during the 'nbdinfo' app when you want to learn as much as possible about every export exposed by a single nbd server, while still remaining in negotiation after each export thus queried. That example is only able to print what NBD_OPT_LIST returns (the export name, and any description the server provides) - some servers might provide the export size in the human-readable description, but the only way to get a machine-readable readout of each size is to further call NBD_OPT_INFO on each name returned by NBD_OPT_LIST. nbd_set_opt_mode() is what lets it be possible to call nbd_opt_list(), but it still requires nbd_opt_list() before you can use nbd_get_size() while still remaining in negotiation mode. The state machine for OPT_GO handles both NBD_OPT_INFO and NBD_OPT_GO under the hood (the two have identical query and response layouts, the only real difference is that the choice of opcode determines whether the state machine transitions on to transmission phase or remains back in negotiation phase). Similarly, the state machine for OPT_META_CONTEXT handles both NBD_OPT_LIST_META_CONTEXT and NBD_OPT_SET_META_CONTEXT (same query and response layouts). For nbd_opt_info(), the second half is NBD_OPT_INFO; for nbd_opt_go(), the second half is NBD_OPT_GO, but both use the same state machine for both halves. NBD_OPT_INFO is useless if you want to do reads or writes from the export, but useful if you want to merely learn information about what multiple exports that the server is letting you choose from. We have been trying to keep the NBD protocol to track as little state as possible. However, at the time we added NBD_CMD_BLOCK_STATUS, the protocol did not have a way to indicate payload size on transmission requests, so the solution in the protocol was to have a single context-to-id stateful mapping set during negotiation by NBD_OPT_SET_META_CONTEXT that is then used for all subsequent NBD_CMD_BLOCK_STATUS; if you negotiate multiple contexts, the server is then obligated to give you an answer for each of those contexts on every block status request, even if your reason for calling block status was to only find out information on just one of those contexts (then again, many clients only negotiate a single context of "base:allocation", so for those clients, it's not as onerous as it sounds). Part of my (ongoing) work at adding 64-bit extensions to the NBD protocol is extending the transmission phase to actually advertise payload lengths from the client request, at which point it DOES become possible to have a variant of NBD_CMD_BLOCK_STATUS where the client can ask for just one context on a given block status request, even though both the client and server are aware of more contexts being available. But whether that filtering should be in the form of passing in a string name (that the server must parse on every NBD_CMD_BLOCK_STATUS which includes a filter payload), or whether the server should rely on its name-to-id mapping set up once during NBD_OPT_SET_META_CONTEXT, is still something we could debate when I post my 64-bit extension patches. [1] As part of my v2 patch, I have written a 2-line nbdkit hack patch that makes nbdkit behave as a server that exposes yet another pre-existing bug in libnbd: if NBD_OPT_SET_META_CONTEXT starts replying with a valid name-to-id mapping (NBD_REP_META_CONTEXT), then fails the overall command for an unrelated reason (NBD_REP_ERR...) rather than concluding with NBD_REP_ACK; the NBD spec says that in this case, the client should assume no name-to-id mapping is valid and NBD_CMD_BLOCK_STATUS should not be attempted; but current libnbd forgets to clear h->meta_contexts state. As mentioned in my musings above about export state being different from context state, it is nice that nbd_can_meta_context() normally returns 0 after nbd_opt_go() for all unknown names, even for a server that lacked NBD_OPT_SET_META_CONTEXT; but I think it makes sense to have nbd_can_meta_context() actively return -1 for all names even after nbd_opt_go() if NBD_OPT_SET_META_CONTEXT failed for any reason other than NBD_REP_ERR_UNSUP (this is a rare server corner case that does not crop up in normal practice - but is worth diagnosing if it does, as with my 2-line hacked nbdkit). It also means I'm also leaning towards: nbd_set_opt_mode(true) nbd_set_request_meta_context(false) nbd_connect_...() nbd_opt_go() nbd_can_meta_context(anything) => -1 where we explicitly indicate that the user asked to take control over context negotiation, but then failed to call nbd_opt_set_meta_context(), and therefore nbd_can_meta_context() should be an error rather than successfully claiming false. I had originally planned nbd_set_request_meta_context() and nbd_opt_set_meta_context() to be added in the same patch, but in what I've written above, I now have sufficient test-case proof that I can separate those API additions into separate patches for easier review of incremental improvements. If that wording survives anywhere into my (larger) v2 series, I like your improved wording and will utilize it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Tue, Aug 30, 2022 at 01:16:02PM +0200, Laszlo Ersek wrote: The trouble with this is that maintaining the non-C language wrappers becomes a huge amount of work that is constantly behind (see also: libvirt). Generating bindings means less work and more utility for everyone. The down-side is that the bindings may not be completely natural in all cases in all languages and there's a bit of a lowest common denominator effect. Despite this, the non-C bindings are still pretty good for libnbd. Maybe in practice everyone is going to use nbdinfo :-) but it's still useful to have a way to query what exports are available. (Same for metadata contexts.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v