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

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&gt; > --- > > 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: