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

[Laszlo Ersek]

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