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