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 <ebl...@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?plain=1#L963
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 <ler...@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
