Re: [Libguestfs] [libnbd PATCH 1/7] api: Expose "qemu:" meta-context constants

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&gt; > --- > 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 do see the last hunk for the interop/ directory -- I'm unsure what that directory is for.)

- 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").

- I *think* this patch is not needed for 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&gt;

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(a)redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs