10:03 a.m.
On 7/27/23 16:33, Eric Blake wrote:
On Thu, Jul 27, 2023 at 01:41:03PM +0200, Laszlo Ersek wrote:
> On 7/26/23 19:29, Eric Blake wrote:
>> Qemu has a couple of documented meta-context definitions[1]; we might
>> as well expose constants for these namespaces.
>>
>> "qemu:dirty-bitmap:NAME" is a set of namespaces for any arbitrary
>> dirty bitmap name; we can't define constants for every bitspace name,
>> but it is possible to do NBD_OPT_LIST_META_CONTEXT on
>> "qemu:dirty-bitmap:" to see which dirty bitmaps are available. When a
>> dirty bitmap is negotiated, only one bit is defined (an extent is
>> dirty or not). The presence of '-' and ':' in the context name
beyond
>> the namespace requires a new helper function.
>>
>> "qemu:allocation-depth" returns an integer rather than a bitmap (0 for
>> unmapped, 1 for current image, 2 and beyond for number of files in the
>> backing chain before the data was supplied), so we can't really define
>> any constants for interpreting its values.
>>
>> [1] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/interop/nbd.txt
>>
>> For libnbd.h, the generated diff is:
>>
>> | --- include/libnbd.h.bak 2023-07-26 11:01:45.401328604 -0500
>> | +++ include/libnbd.h 2023-07-26 11:59:38.289021067 -0500
>> | @@ -1083,6 +1083,16 @@
>> | #define LIBNBD_STATE_HOLE 1
>> | #define LIBNBD_STATE_ZERO 2
>> |
>> | +/* "qemu" namespace */
>> | +#define LIBNBD_NAMESPACE_QEMU "qemu:"
>> | +
>> | +/* "qemu" namespace contexts */
>> | +#define LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP "qemu:dirty-bitmap:"
>> | +#define LIBNBD_CONTEXT_QEMU_ALLOCATION_DEPTH
"qemu:allocation-depth"
>> | +
>> | +/* "qemu:dirty-bitmap:" context related constants */
>> | +#define LIBNBD_STATE_DIRTY 1
>> | +
>> | #ifdef __cplusplus
>> | }
>> | #endif
>>
>> Signed-off-by: Eric Blake <eblake(a)redhat.com>
>> ---
>> generator/utils.mli | 1 +
>> generator/API.ml | 9 ++++++---
>> generator/C.ml | 22 +++++++++++++++-------
>> generator/GoLang.ml | 3 ++-
>> generator/OCaml.ml | 4 ++--
>> generator/Python.ml | 3 ++-
>> generator/utils.ml | 5 +++++
>> interop/dirty-bitmap.c | 6 ++++--
>> 8 files changed, 37 insertions(+), 16 deletions(-)
>
> I'm not really convinced this is helpful.
>
> - Libnbd is not supposed to be tied to any particular NBD server AIUI,
> so open-coding QEMU-specific constants in the library header (for which
> we promise stability, in C) seems unneeded and risky.
I'm of the opinion that if any project declares their own namespace of
extensions, and then documents that declaration in the upstream NBD
spec (which qemu has done: [1]), then libnbd might as well try and
make it easier to probe for the existence of that extension.
[1]
https://github.com/NetworkBlockDevice/nbd/blob/58b356bd19e63/doc/proto.md...
Good point! I didn't realize the QEMU extensions were (effectively)
standardized in the spec. That makes my first point moot.
(More importantly -- I didn't realize there was a *registration process*
in place! That's a great thing to have.)
>
> (I do see the last hunk for the interop/ directory -- I'm unsure what
> that directory is for.)
That provides interoperability tests of what libnbd does when talking
to various servers (some of the tests are repeated across nbdkit,
qemu, and nbd-server; some are specific to features of a given
server). Altering the test to actually use the constants defined by
this patch ensures that we have API stability for those constants.
Yes.
>
> - In the other direction, we don't implement the whole story (for
> "qemu:allocation-depth"). In theory, we could introduce symbolic
> constants for 0 and 1, such as LIBNBD_STATE_UNMAPPED and
> LIBNBD_STATE_CURRENT (and client code could use
">LIBNBD_STATE_CURRENT",
> i.e., ">1", equivalently to ">=2").
Yes, we could indeed add at least 0 and 1 as constants for
qemu:allocation-depth, if we wanted. I'm not sure how to go about
documenting them, though; the point is that base:allocation uses flags
as a bitmap, but qemu:allocation-depth uses flags as an integer.
Anyway, we can always do that later.
>
> - I *think* this patch is not needed for patch#2.
Correct. This patch is independent of the Go changes, but noticed
because the way we were outputting a list of meta-contexts when the
list was only 1 element long was odd. Making the list more than one
element long made it easier to test that all the loops are working
correctly (or, as shown by this patch, that reordering the loops makes
output more sensible).
This in itself is not enough to justify patch#1, however, with the QEMU
extensions being listed in the NBD spec, patch#1 is a nice benefit for
testing another aspect of patch#2.
>
> Anyway, I don't want to block this patch just because I'm not convinced
> enough to review it in detail :) So if Rich likes it, I'm certainly game.
>
> Acked-by: Laszlo Ersek <lersek(a)redhat.com>
I'll wait to see what Rich suggests.
FWIW, the qemu meta-context namespace being registered in the spec has
warmed me up to this patch!
Laszlo