On 08/25/22 17:05, Eric Blake wrote:
On Thu, Aug 25, 2022 at 12:13:42PM +0200, Laszlo Ersek wrote:
[...]
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).
Thanks for the explanation. I didn't expect these two principles to have driven the design (generability, and long-term convenience calls). I think the more usual (albeit likely less programmer-friendly) approach is to (a) expose the minute details and let the end-programmer deal with them, (b) let people write / maintain other-than-C language wrappers manually.
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, ...) [...] context mappings (nbd_can_meta_context) [...]
This is a huge amount of nuances -- I remember being in a position where, as a long-term maintainer, I could collect, or at least understand, this many bits intuitively, but that doesn't work for me as a newcomer to a project. So please try to write surgical, super-focused patches, if I'm to have a chance at reviewing them. :)
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).
I don't understand why this is useful, as a part of the protocol. It seems difficult to program a robust client if the server is permitted to respond in so different styles! Maybe the output of this operation was not (primarily) meant to be consumed by a program (but humans that are happy to iterate and to construct new queries on the spot).
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".
Hmm, OK.
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.
Given that we need to keep the old APIs for compatibility's sake, I think new APIs should not be introduced unless they add a new capability. There is already a multitude of APIs.
- NBD_OPT_INFO in particular looks like a useless opcode.
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.
Such "discovery" looks useful for humans, but is it useful to client programs other than nbdinfo?
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>.
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()
(s/nbd_opt_list/nbd_opt_info/ ?)
while still remaining in negotiation mode.
Aha! So the trick is that the example code uses nbd_get_size() *after* nbd_opt_go(), but we may want to call nbd_get_size() before entering the "ready" state. [...] I've read the rest too -- it's a lot to digest. I've nothing more to add at this moment. Thank you! Laszlo