On Thu, Jul 27, 2023 at 09:33:19AM -0500, 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
>
> >
> > (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.
Sorry for the delay. Looks like the patch is upstream, and
it seems fine to me.
Rich.
> >
> > 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
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
