6:16 a.m.
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