Libguestfs - Jun 2019 - Re: [libnbd PATCH] generator: Add support for namespace constants

This just defines the namespace, its contexts and related constants and the only
supported one is currently base:allocation.  The names of the constants are not
very future-proof, but shorter than LIBNBD_META_NS_CONTEXT_BASE_ALLOCATION or
similar.

Currently the output looks like this:

/* "base" namespace */

/* "base" namespace contexts */

/* "base:allocation" context related constants */

Separated by two empty lines from unrelated parts of the header file above and
below.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---

Notes:
    Everything is up for a debate:
    
    - the names might need to be different so that they do not clash with other
      constants in the scope later on,
    
    - the fact that "base" and "base:allocation" are even
defined, which might be
      useless, since listing contexts of a namespace is not exposed,
    
    - whether this should live in a separate (still included in libnbd.h) file,
    
    - and more...

 generator/generator | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/generator/generator b/generator/generator
index 684fb44ac918..9d8d257fd2f1 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2066,6 +2066,13 @@ let constants = [
   "READ_ERROR",          3;
 ]
 
+let metadata_namespaces = [
+  "base", [ "allocation", [
+    "STATE_HOLE", 1 lsl 0;
+    "STATE_ZERO", 1 lsl 1;
+  ] ];
+]
+
 (*----------------------------------------------------------------------*)
 
 (* Helper functions. *)
@@ -2908,6 +2915,25 @@ let print_extern_and_define name args ret    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
+  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
+
+let print_ns ns ctxts +  let ns_upper = String.uppercase_ascii ns in
+  pr "/* \"%s\" namespace */\n" ns;
+  pr "#define LIBNBD_NAMESPACE_%s \"%s\"\n" ns_upper ns;
+  pr "\n";
+  pr "/* \"%s\" namespace contexts */\n" ns;
+  List.iter (
+    fun (ctxt, consts) -> print_ns_ctxt ns ns_upper ctxt consts
+  ) ctxts;
+  pr "\n"
+
 let generate_include_libnbd_h ()    generate_header CStyle;
 
@@ -2944,6 +2970,10 @@ let generate_include_libnbd_h ()      fun (name, { args;
ret }) -> print_extern_and_define name args ret
   ) handle_calls;
   pr "\n";
+  List.iter (
+    fun (ns, ctxts) -> print_ns ns ctxts
+  ) metadata_namespaces;
+  pr "\n";
   pr "#endif /* LIBNBD_H */\n"
 
 let generate_lib_unlocked_h () -- 
2.22.0

Thanks - I have pushed this.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 6/27/19 5:07 AM, Martin Kletzander wrote:
> This just defines the namespace, its contexts and related constants and the
only
> supported one is currently base:allocation.  The names of the constants are
not
> very future-proof, but shorter than LIBNBD_META_NS_CONTEXT_BASE_ALLOCATION
or
> similar.
> 
> Currently the output looks like this:
> 
> /* "base" namespace */
> 
> /* "base" namespace contexts */
> 
> /* "base:allocation" context related constants */
> 
> Separated by two empty lines from unrelated parts of the header file above
and
> below.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> 
> Notes:
>     Everything is up for a debate:
>     
>     - the names might need to be different so that they do not clash with
other
>       constants in the scope later on,
Yeah, could be a problem. We have until API stability freeze to change
our minds.  I'm guessing we might need:

LIBNBD_CONTEXT_BASE_ALLOCATION_STATE_HOLE
LIBNBD_CONTEXT_BASE_ALLOCATION_STATE_ZERO

but yes, that's a mouthful to type. Keeping the short names
LIBNBD_STATE_HOLE for now is okay.
>     
>     - the fact that "base" and "base:allocation" are
even defined, which might be
>       useless, since listing contexts of a namespace is not exposed,
Rich still has the idea of adding a 'qemu-nbd --list' counterpart, so
defining a constant for the "base:" and "qemu:" namespaces
makes sense
for that even if we can't use it now.  Hmm - your patch defines
LIBNBD_NAMESPACE_BASE to "base", but in practice we'd want to pass
"base:" when querying which contexts are available in that namespace.
>     
>     - whether this should live in a separate (still included in libnbd.h)
file,
Single file is fine by me for now; we can always split later if it gets
huge.
>     
>     - and more...
I'd love to have LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP(foo) produce the
string "qemu:dirty-bitmap:foo". I'm not sure how to wire that in
on top
of your patches, so it doesn't have to happen today, but it's food for
thought.

Here's what I'm pushing as a followup to your patch, to add more
documentation about the constants, and to use them in our testsuite:

diff --git i/generator/generator w/generator/generator
index 9d8d257..1cfcb3d 100755
--- i/generator/generator
+++ w/generator/generator
@@ -1118,7 +1118,10 @@ was actually negotiated, call
C<nbd_can_meta_context> after
 connecting.

 The single parameter is the name of the metadata context,
-for example C<\"base:allocation\">.
+for example C<LIBNBD_CONTEXT_BASE_ALLOCATION>.
+B<E<lt>libnbd.hE<gt>> includes defined constants beginning
with
+C<LIBNBD_CONTEXT_> for some well-known contexts, but you are free
+to pass in other contexts.

 Other metadata contexts are server-specific, but include
 C<\"qemu:dirty-bitmap:...\"> for qemu-nbd
@@ -1293,7 +1296,10 @@ Returns true if the server supports the given
meta context
 the server does not.

 The single parameter is the name of the metadata context,
-for example C<\"base:allocation\">.";
+for example C<LIBNBD_CONTEXT_BASE_ALLOCATION>.
+B<E<lt>libnbd.hE<gt>> includes defined constants for
well-known
+namespace contexts beginning with C<LIBNBD_CONTEXT_>, but you
+are free to pass in other contexts.";
   };

   "get_size", {
@@ -1551,7 +1557,9 @@ pair being the length (in bytes) of the block and
the second entry
 being a status/flags field which is specific to the metadata context.
 (The number of pairs passed to the function is C<nr_entries/2>.)  The
 NBD protocol document in the section about
-C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array.
+C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array;
+for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains
constants
+beginning with C<LIBNBD_STATE_> that may help decipher the values.

 It is possible for the extent function to be called
 more times than you expect (if the server is buggy),
@@ -2926,7 +2934,7 @@ let print_ns_ctxt ns ns_upper ctxt consts  let print_ns ns
ctxts    let ns_upper = String.uppercase_ascii ns in
   pr "/* \"%s\" namespace */\n" ns;
-  pr "#define LIBNBD_NAMESPACE_%s \"%s\"\n" ns_upper ns;
+  pr "#define LIBNBD_NAMESPACE_%s \"%s:\"\n" ns_upper ns;
   pr "\n";
   pr "/* \"%s\" namespace contexts */\n" ns;
   List.iter (
diff --git i/interop/dirty-bitmap.c w/interop/dirty-bitmap.c
index 61661fa..329fbea 100644
--- i/interop/dirty-bitmap.c
+++ w/interop/dirty-bitmap.c
@@ -30,7 +30,6 @@

 static const char *unixsocket;
 static const char *bitmap;
-static const char *base_allocation = "base:allocation";

 struct data {
   bool req_one;    /* input: true if req_one was passed to request */
@@ -53,7 +52,7 @@ cb (void *opaque, const char *metacontext, uint64_t
offset,
   assert (offset == 0);
   assert (data->count-- > 0); /* [qemu-nbd] */

-  if (strcmp (metacontext, base_allocation) == 0) {
+  if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) {
     assert (!data->seen_base); /* [qemu-nbd] */
     data->seen_base = true;
     assert (len == (data->req_one ? 2 : 8)); /* [qemu-nbd] */
@@ -62,11 +61,13 @@ cb (void *opaque, const char *metacontext, uint64_t
offset,
     assert (entries[0] == 131072); assert (entries[1] == 0);
     if (!data->req_one) {
       /* hole|zero offset 128k size 384k */
-      assert (entries[2] == 393216); assert (entries[3] == 3);
+      assert (entries[2] == 393216); assert (entries[3] =(LIBNBD_STATE_HOLE|
+
LIBNBD_STATE_ZERO));
       /* allocated zero offset 512k size 64k */
-      assert (entries[4] ==  65536); assert (entries[5] == 2);
+      assert (entries[4] ==  65536); assert (entries[5] =LIBNBD_STATE_ZERO);
       /* hole|zero offset 576k size 448k */
-      assert (entries[6] == 458752); assert (entries[7] == 3);
+      assert (entries[6] == 458752); assert (entries[7] =(LIBNBD_STATE_HOLE|
+
LIBNBD_STATE_ZERO));
     }
   }
   else if (strcmp (metacontext, bitmap) == 0) {
@@ -117,7 +118,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  nbd_add_meta_context (nbd, base_allocation);
+  nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);
   nbd_add_meta_context (nbd, bitmap);

   if (nbd_connect_unix (nbd, unixsocket) == -1) {
diff --git i/tests/meta-base-allocation.c w/tests/meta-base-allocation.c
index b00aa50..a9ddbd0 100644
--- i/tests/meta-base-allocation.c
+++ w/tests/meta-base-allocation.c
@@ -56,7 +56,7 @@ main (int argc, char *argv[])
   /* Negotiate metadata context "base:allocation" with the server.
    * This is supported in nbdkit >= 1.12.
    */
-  if (nbd_add_meta_context (nbd, "base:allocation") == -1) {
+  if (nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -85,7 +85,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  switch (nbd_can_meta_context (nbd, "base:allocation")) {
+  switch (nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) {
   case -1:
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
@@ -137,7 +137,7 @@ check_extent (void *data, const char *metacontext,
           "nr_entries=%zu\n",
           id, metacontext, offset, nr_entries);

-  if (strcmp (metacontext, "base:allocation") == 0) {
+  if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) {
     for (i = 0; i < nr_entries; i += 2) {
       printf ("\t%zu\tlength=%" PRIu32 ", status=%" PRIu32
"\n",
               i, entries[i], entries[i+1]);
@@ -148,21 +148,24 @@ check_extent (void *data, const char *metacontext,
     case 1:
       assert (nr_entries == 10);
       assert (entries[0] == 8192);  assert (entries[1] == 0);
-      assert (entries[2] == 8192);  assert (entries[3] == 1);
-      assert (entries[4] == 16384); assert (entries[5] == 3);
-      assert (entries[6] == 16384); assert (entries[7] == 2);
+      assert (entries[2] == 8192);  assert (entries[3] =LIBNBD_STATE_HOLE);
+      assert (entries[4] == 16384); assert (entries[5] =(LIBNBD_STATE_HOLE|
+
LIBNBD_STATE_ZERO));
+      assert (entries[6] == 16384); assert (entries[7] =LIBNBD_STATE_ZERO);
       assert (entries[8] == 16384); assert (entries[9] == 0);
       break;

     case 2:
       assert (nr_entries == 4);
-      assert (entries[0] == 512);   assert (entries[1] == 3);
-      assert (entries[2] == 16384); assert (entries[3] == 2);
+      assert (entries[0] == 512);   assert (entries[1] =(LIBNBD_STATE_HOLE|
+
LIBNBD_STATE_ZERO));
+      assert (entries[2] == 16384); assert (entries[3] =LIBNBD_STATE_ZERO);
       break;

     case 3:
       assert (nr_entries == 2);
-      assert (entries[0] == 512);   assert (entries[1] == 3);
+      assert (entries[0] == 512);   assert (entries[1] =(LIBNBD_STATE_HOLE|
+
LIBNBD_STATE_ZERO));
       break;

     default:


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Jun 27, 2019 at 09:25:38AM -0500, Eric Blake
wrote:
>On 6/27/19 5:07 AM, Martin Kletzander wrote:
>> This just defines the namespace, its contexts and related constants and
the only
>> supported one is currently base:allocation.  The names of the constants
are not
>> very future-proof, but shorter than
LIBNBD_META_NS_CONTEXT_BASE_ALLOCATION or
>> similar.
>>
>> Currently the output looks like this:
>>
>> /* "base" namespace */
>>
>> /* "base" namespace contexts */
>>
>> /* "base:allocation" context related constants */
>>
>> Separated by two empty lines from unrelated parts of the header file
above and
>> below.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>
>> Notes:
>>     Everything is up for a debate:
>>
>>     - the names might need to be different so that they do not clash
with other
>>       constants in the scope later on,
>
>Yeah, could be a problem. We have until API stability freeze to change
>our minds.  I'm guessing we might need:
>
>LIBNBD_CONTEXT_BASE_ALLOCATION_STATE_HOLE
>LIBNBD_CONTEXT_BASE_ALLOCATION_STATE_ZERO
>
>but yes, that's a mouthful to type. Keeping the short names
>LIBNBD_STATE_HOLE for now is okay.
>
I forgot to say I also went with that naming as NBD_STATE_{HOLE,ZERO} are
defined and spelled that way in the NBD protocol spec.
>>
>>     - the fact that "base" and "base:allocation"
are even defined, which might be
>>       useless, since listing contexts of a namespace is not exposed,
>
>Rich still has the idea of adding a 'qemu-nbd --list' counterpart,
so
>defining a constant for the "base:" and "qemu:"
namespaces makes sense
>for that even if we can't use it now.  Hmm - your patch defines
>LIBNBD_NAMESPACE_BASE to "base", but in practice we'd want to
pass
>"base:" when querying which contexts are available in that
namespace.
>
That is on the protocol, but the API does not need to take the ':*'
suffix.
Also it is costless to do LIBNBD_NAMESPACE_BASE ":" in the C code.
>>
>>     - whether this should live in a separate (still included in
libnbd.h) file,
>
>Single file is fine by me for now; we can always split later if it gets
>huge.
>
>>
>>     - and more...
>
>I'd love to have LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP(foo) produce the
>string "qemu:dirty-bitmap:foo". I'm not sure how to wire that
in on top
>of your patches, so it doesn't have to happen today, but it's food
for
>thought.
>
You mean, for example:

#define LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP(foo) "qemu:dirty-bitmap:"
#foo

Although I'm not sure about all the corner cases, like if there can be (or
need
to be) parentheses around the result.
>Here's what I'm pushing as a followup to your patch, to add more
>documentation about the constants, and to use them in our testsuite:
>
Oh yes, I completely missed that.  Not only have I meant this as kind of an RFC,
but I also completely missed the documentation and tests.  So thanks a lot for
that fix.

Martin