5:07 a.m.
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(a)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
7:22 a.m.
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/
9:25 a.m.
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(a)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
1:32 a.m.
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(a)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
10:50 a.m.
On 6/28/19 1:32 AM, Martin Kletzander wrote:
> 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.
With my patch, it is currently "base:" in the header (and in the other
language bindings). I suggest that until we actually have an API for
doing listings, it doesn't really matter, we can change it to drop the
trailing ':' at that time if it proves easier to force clients to do the
string constant concatenation for using the new API at that time.
>
> 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
I guess it depends on whether call-sites will look like
LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP("foo")
(performing constant string concatenation)
vs.
LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP(foo)
(performing runtime dynamic computation)
so the macro may have to call out to a function and reserve thread-local
storage. It may be another thing where we tweak things based on what's
easiest to implement and easiest for clients to consume.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
1855
days inactive
1856
days old
4 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Richard W.M. Jones