[libvirt] [libvirt RFC PATCH 02/10] util: storage: Add parser for qemu's "json" backing pseudo-protocol

[Peter Krempa]

On Tue, Jul 26, 2016 at 14:28:23 -0600, Eric Blake wrote:
> On 07/15/2016 07:46 AM, Peter Krempa wrote:
> > Add a modular parser that will allow to parse 'json' backing definitions
> > that are supported by qemu. The initial implementation adds support for
> > the 'file' driver.
> 
> Might be nice for this commit message to show an actual json: string
> that it now accepts.
> 
> > ---
> >  src/util/virstoragefile.c | 87 +++++++++++++++++++++++++++++++++++++++++++----
> >  tests/virstoragetest.c    |  8 +++++
> >  2 files changed, 88 insertions(+), 7 deletions(-)
> > 
> 
> At some point, I wonder if we can use qemu's QMP introspection to write
> a meta-modular parser (basically, once QMP blockdev-add is
> fully-functional, introspection will give a self-describing layout of
> all possible combinations that QMP will accept, and therefore parsing
> the introspection output would give a reasonable approach to all
> possible json: strings that a given qemu will understand).  But that's
> definitely more work, and blockdev-add is not fully working in qemu yet,
> so hand-duplicating common parses for now at least gets us further than
> libvirt's current stance of outright choking on json:.
> 
> 
> > @@ -2514,10 +2515,80 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
> >  }
> > 
> > 
> > +static int
> > +virStorageSourceParseBackingJSONPath(virStorageSourcePtr src,
> > +                                     virJSONValuePtr json,
> > +                                     int type)
> > +{
> > +    const char *path;
> > +
> > +    if (!(path = virJSONValueObjectGetString(json, "file.filename"))) {
> > +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> > +                       _("missing 'filename' field in JSON backing volume "
> > +                         "definition"));
> 
> Do we really want to require libvirt to support
> "file.filename":"/path/to/foo" compressed naming, or do we want a more

The problem is that once you allow something in qemu there are users
which actually will start using it. And that happened.

> hierarchical "file":{"filename":"/path/to/foo"} layout?  Or should both

Yes I'll switch to this for the main parser and all the weird layouts
will be mapped first to this.

> be supported?  I also wonder if the virJSON code should be enhanced to

So I think we will need to support both.

> make it easier to parse deep information, via something similar to xpath
> notation (other shortcuts that might be nice is "key[0]" as shorthand
> for accessing element0 out of "key":[element0, element1]).
> 
> Part of the problem is that qemu is doing so much gross under-the-hood

And the other part is lack of documentation for that. This also makes
users try what works and use that rather than the preferred syntax.

> conversion from pure QAPI into QemuOpts (which is completely flat, so we
> have hacks like "file.filename"), rather than converting from QemuOpts
> into QAPI, and so now libvirt has to match that grossness.

And since both approaches seem to have attacted their users qemu will
need to support it forever.

One other weird part is that the parser in qemu(-img) parses the string
but then puts it in the user supplied format into the qcow image. Any
extra spaces and other stuff are included. By using the formatter for
that all the weird syntax could be avoided.

> 
> > +++ b/tests/virstoragetest.c
> > @@ -1353,6 +1353,14 @@ mymain(void)
> >                         "<source protocol='nbd' name='blah'>\n"
> >                         "  <host name='example.org' port='6000'/>\n"
> >                         "</source>\n");
> > +    TEST_BACKING_PARSE(5, "json:", NULL);
> > +    TEST_BACKING_PARSE(6, "json:asdgsdfg", NULL);
> > +    TEST_BACKING_PARSE(7, "json:{}", NULL);
> > +    TEST_BACKING_PARSE(8, "json: { \"file.driver\":\"blah\"}", NULL);
> > +    TEST_BACKING_PARSE(9, "json:{\"file.driver\":\"file\"}", NULL);
> > +    TEST_BACKING_PARSE(10, "json:{\"file.driver\":\"file\", "
> > +                                 "\"file.filename\":\"/path/to/file\"}",
> > +                       "<source file='/path/to/file'/>\n");
> > 
> 
> Okay, the testsuite shows what you now parse.  Again, I'm wondering if
> qemu should first be improved, and/or whether it already allows:
> 
> "json:{\"file\":{\"driver\":\"file\", \"filename\":\"/path/to/file\"}}"
> 
> as equivalent to the "file.driver" and "file.filename" at the flat
> level.  Or put another way, if qemu accepts more than one JSON
> representation because of the way it crumples and flattens JSON into
> QDict, maybe libvirt should first have a way to canonicalize into
> preferred form.

Yep, the nested objects will be preferred in this case as others are
weird syntax can be adapted.

Since this RFC was already reused in a different series I'll fix the
parser first and then re-post it.

Peter