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 > > (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. > > - 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. > > - 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). > > 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. > > Thanks > Laszlo > > > > > > diff --git a/generator/utils.mli b/generator/utils.mli > > index 773e705f..c4d47a34 100644 > > --- a/generator/utils.mli > > +++ b/generator/utils.mli > > @@ -46,6 +46,7 @@ val > > val cspan : string -> string -> int > > val quote : string -> string > > val spaces : int -> string > > +val macro_name : string -> string > > val files_equal : string -> string -> bool > > > > val generate_header : > > diff --git a/generator/API.ml b/generator/API.ml > > index 02c1260d..72c81657 100644 > > --- a/generator/API.ml > > +++ b/generator/API.ml > > @@ -3919,9 +3919,12 @@ let constants = > > > > let metadata_namespaces = [ > > "base", [ "allocation", [ > > - "STATE_HOLE", 1 lsl 0; > > - "STATE_ZERO", 1 lsl 1; > > - ] ]; > > + "STATE_HOLE", 1 lsl 0; > > + "STATE_ZERO", 1 lsl 1; > > + ] ]; > > + "qemu", [ "dirty-bitmap:", [ "STATE_DIRTY", 1 lsl 0; ]; > > + "allocation-depth", []; > > + ]; > > ] > > > > let pod_of_link = function > > diff --git a/generator/C.ml b/generator/C.ml > > index f772117c..a2670f70 100644 > > --- a/generator/C.ml > > +++ b/generator/C.ml > > @@ -373,13 +373,18 @@ let > > pr "#define LIBNBD_HAVE_NBD_%s 1\n" name_upper; > > pr "\n" > > > > -let print_ns_ctxt ns ns_upper ctxt consts = > > - let ctxt_upper = String.uppercase_ascii ctxt in > > +let print_ns_ctxt ns ns_upper ctxt = > > + let ctxt_macro = macro_name ctxt in > > + let ctxt_upper = String.uppercase_ascii ctxt_macro in > > pr "#define LIBNBD_CONTEXT_%s_%s \"%s:%s\"\n" > > - ns_upper ctxt_upper ns ctxt; > > - pr "\n"; > > - pr "/* \"%s:%s\" context related constants */\n" ns ctxt; > > - List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts > > + ns_upper ctxt_upper ns ctxt > > + > > +let print_ns_ctxt_bits ns ctxt consts = > > + if consts <> [] then ( > > + pr "\n"; > > + pr "/* \"%s:%s\" context related constants */\n" ns ctxt; > > + List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts > > + ) > > > > let print_ns ns ctxts = > > let ns_upper = String.uppercase_ascii ns in > > @@ -388,7 +393,10 @@ let > > pr "\n"; > > pr "/* \"%s\" namespace contexts */\n" ns; > > List.iter ( > > - fun (ctxt, consts) -> print_ns_ctxt ns ns_upper ctxt consts > > + fun (ctxt, _) -> print_ns_ctxt ns ns_upper ctxt > > + ) ctxts; > > + List.iter ( > > + fun (ctxt, consts) -> print_ns_ctxt_bits ns ctxt consts > > ) ctxts; > > pr "\n" > > > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > > index 50ed7226..7a7e7f4b 100644 > > --- a/generator/GoLang.ml > > +++ b/generator/GoLang.ml > > @@ -472,7 +472,8 @@ let > > pr " namespace_%s = \"%s:\"\n" ns ns; > > List.iter ( > > fun (ctxt, consts) -> > > - pr " context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; > > + let ctxt_macro = macro_name ctxt in > > + pr " context_%s_%s = \"%s:%s\"\n" ns ctxt_macro ns ctxt; > > List.iter (fun (n, v) -> > > pr " %s uint32 = %d\n" n v > > ) consts > > diff --git a/generator/OCaml.ml b/generator/OCaml.ml > > index edb81f25..efc1af22 100644 > > --- a/generator/OCaml.ml > > +++ b/generator/OCaml.ml > > @@ -183,7 +183,7 @@ type > > pr "val namespace_%s : string\n" ns; > > List.iter ( > > fun (ctxt, consts) -> > > - pr "val context_%s_%s : string\n" ns ctxt; > > + pr "val context_%s_%s : string\n" ns (macro_name ctxt); > > List.iter (fun (n, _) -> > > pr "val %s : int32\n" (String.lowercase_ascii n) > > ) consts > > @@ -315,7 +315,7 @@ let > > pr "let namespace_%s = \"%s:\"\n" ns ns; > > List.iter ( > > fun (ctxt, consts) -> > > - pr "let context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; > > + pr "let context_%s_%s = \"%s:%s\"\n" ns (macro_name ctxt) ns > > ctxt; > > List.iter (fun (n, i) -> > > pr "let %s = %d_l\n" (String.lowercase_ascii n) i > > ) consts > > diff --git a/generator/Python.ml b/generator/Python.ml > > index c81878de..beaeaa4c 100644 > > --- a/generator/Python.ml > > +++ b/generator/Python.ml > > @@ -745,7 +745,8 @@ let > > pr "NAMESPACE_%s = \"%s:\"\n" ns_upper ns; > > List.iter ( > > fun (ctxt, consts) -> > > - let ctxt_upper = String.uppercase_ascii ctxt in > > + let ctxt_macro = macro_name ctxt in > > + let ctxt_upper = String.uppercase_ascii ctxt_macro in > > pr "%s = \"%s:%s\"\n" > > (sprintf "CONTEXT_%s_%s" ns_upper ctxt_upper) ns ctxt; > > List.iter (fun (n, i) -> pr "%s = %d\n" n i) consts > > diff --git a/generator/utils.ml b/generator/utils.ml > > index e0c67d20..61cce877 100644 > > --- a/generator/utils.ml > > +++ b/generator/utils.ml > > @@ -151,6 +151,11 @@ let > > > > let spaces n = String.make n ' ' > > > > +(* Convert s to macro name by changing '-' to '_' and eliding ':'. *) > > +let macro_name s = > > + let underscore = Str.global_replace (Str.regexp_string "-") "_" s in > > + Str.global_replace (Str.regexp ":") "" underscore > > + > > (* Save the current output channel and replace it with a temporary buffer > > while > > * running ‘code’. Return the buffer. > > *) > > diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c > > index 05f6e9db..16faaed9 100644 > > --- a/interop/dirty-bitmap.c > > +++ b/interop/dirty-bitmap.c > > @@ -127,8 +127,10 @@ main (int argc, char *argv[]) > > nbd_extent_callback extent_callback = { .callback = cb, .user_data = > > &data }; > > char c; > > > > - if (argc < 3) { > > - fprintf (stderr, "%s bitmap qemu-nbd [args ...]\n", argv[0]); > > + if (argc < 3 || strncmp (argv[1], LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP, > > + strlen (LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP)) != > > 0) { > > + fprintf (stderr, "%s qemu:dirty-bitmap:name qemu-nbd [args ...]\n", > > + argv[0]); > > exit (EXIT_FAILURE); > > } > > bitmap = argv[1]; > > > > > > _______________________________________________ > > Libguestfs mailing list > > Libguestfs@redhat.com > > https://listman.redhat.com/mailman/listinfo/libguestfs > -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
