Issue 90 Further Clarifications
Laine Stump
laine at redhat.com
Fri Nov 20 21:22:31 UTC 2020
On 11/20/20 1:07 PM, Dustan B Helm wrote:
> On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa at redhat.com
> <mailto:pkrempa at redhat.com>> wrote:
>
> On Fri, Nov 20, 2020 at 09:41:44 +0000, Daniel Berrange wrote:
> > On Thu, Nov 19, 2020 at 03:48:48PM -0600, Dustan B Helm wrote:
> > > [image: Dustan Helm] <https://gitlab.com/dustan.helm
> <https://gitlab.com/dustan.helm>>
> > > Dustan Helm <https://gitlab.com/dustan.helm
> <https://gitlab.com/dustan.helm>> @dustan.helm
> > > <https://gitlab.com/dustan.helm
> <https://gitlab.com/dustan.helm>> · just now
> > > <https://gitlab.com/libvirt/libvirt/-/issues/90#note_451306432
> <https://gitlab.com/libvirt/libvirt/-/issues/90#note_451306432>>
> > > <https://gitlab.com/libvirt/libvirt/-/issues/90#
> <https://gitlab.com/libvirt/libvirt/-/issues/90#>>
> > >
> > > Before we start making changes and solidifying our XML
> parameter choices,
> > > we have a few clarifying questions about the issue we'd like
> to get out of
> > > the way.
> > >
> > > 1.
> > >
> > > In src/qemu/qemu_capabilities.h, we found the string "/* -drive
> > > file.driver=vxhs via query-qmp-schema */" after the
> QEMU_CAPS_VXHS
> > > declaration. What is the purpose of these strings, and how
> do we modify
> > > them to make sense for nfs? Would we simply mirror what is
> done for VXHS,
> > > adding nfs as the protocol instead?
> >
> > These comments are just a hint/reminder to readers about what
> QEMU command
> > line option(s), the capability check is tracking the
> availability of. That
> > particular example might be a bit misleading, since in the
> qemu_capabilities.c
> > file, we're actually looking for the blockdev-arg/arg-type/+vxhs
> feature,
> > not -drive. QEMU has 2 ways of configuring disks, -drive is the
> historical
> > main way, and -blockdev is the modern way that libvirt
> introduced support
> > for relatively recently. We actually end up having to support
> both approachs
> > in libvirt currrently, as we try to make libvirt work with both
> old and new
> > QEMU versions.
>
> For new features though I strongly prefer if we no longer update
> the old
> code. Implement the new protocol only for -blockdev.
>
> > Peter can probably offer better suggestions than me about what
> specific
> > thing to probe for 'nfs'.
>
> The simplest way to probe for nfs protocol support is to use the
> following query string in virQEMUCapsQMPSchemaQueries[]:
>
> "blockdev-add/arg-type/+nfs"
>
> Looking at the @BlockdevOptionsNfs struct in
> qemu.git/qapi/block-core.json it seems that all properties were
> introduced at same time, so the check doesn't need to be more
> specific.
>
> I can provide more insight on how virQEMUCapsQMPSchemaQueries[]
> works if
> you are interested, but the above will work.
>
> > > 2.
> > >
> > > Where is domain XML parsed and formatted? Is that what is
> referred to by
> > > the schema formats in domaincommon.rng?
> >
> > The domaincommon.rng file provides the RelaxNG schema, which is
> used for
> > (optionally) validating XML files before parsing.
> >
> > The actual parser lives in src/conf/domain_conf.{c,h} files.
> >
> > There are also docs for users about the schema in
> docs/formatdomain.rst
> >
>
> Adding the NFS protocol itself is rather trivial because we use enum
> to string convertors which cause a compilation failure if you don't
> populate the strings, so adding the protocol type will be enough to
> figure out where.
>
> If you want to implement other properties of the nfs protocol driver
> such as @user or @group. I suggest you first send a RFC mail with your
> proposed XML addition for review before diving into the rng schema and
> XML/formatter parser.
>
> Looking at the options in @BlockdevOptionsNfs @user and @group seem a
> bit interesting. I'd not worry with the rest probably.
>
> > > 3.
> > >
> > > In src/qemu/qemu_block.c, the json object arguments
> currently present in
> > > qemuBlockStorageSourceGetVxHSProps(...) are not the same
> ones listed in the
> > > example commit. What is the reason for this change, and how
> should we take
> > > it into account when implementing a new protocol type?
> >
> > I'll leave thi question to Peter too.
>
> I'm not going to outline why we've changed the old commit. You are
> implementing new code. You'll need to add a handler to
> qemuBlockStorageSourceGetBackendProps which converts the appropriate
> fields of virStorageSource to a virJSONValue object which maps to the
> qemu properties according to the @BlockdevOptionsNfs qemu struct.
>
> To verify that it's correct you can add a TEST_DISK_TO_JSON case to
> tests/qemublocktest.c (input files are in
> libvirt/tests/qemublocktestdata/xml2json/ ) where you provide a
> disk XML
> snippet and the output is what we'd use with qemu.
>
> Note that the 'onlytarget' boolean formats a string which is written
> into the qcow2 header file if you create an overlay/external snapshot,
> so it must not include any transient or authentication data.
>
> All of the above is QMP-schema validated so you'll be notified if it
> doesn't conform to qemu's monitor schema.
>
> You'll also need to implement a backing store string parser (that is
> the string which qemu writes to the overlay as noted above). The
> parser
> entry point is virStorageSourceParseBackingJSON and the backends
> for it
> are registered in virStorageSourceJSONDriverParser struct.
>
> The tests for the above are in tests/virstoragetest.c as
> TEST_BACKING_PARSE.
>
> Then depending on whether you actually want to add support for image
> creation (e.g. to support creating snapshots backed by the NFS storage
> directly) you then need to implement hanling in
> qemuBlockStorageSourceCreateGetStorageProps.
>
> You'll definitely see all the places that might need implementing once
> you add the new protocol entry to enum virStorageNetProtocol as we in
> many cases use switch statements with proper type where the compiler
> reminds you that you need to add the handling for the new value in the
> given patch.
>
> Since implementation of the qemu bits should be in a separate commit
> from the one adding the parser bits and thus the new enum, it's
> okay to
> just add the enum value to the swithc case and implement it later.
>
> We weren't exactly sure what you meant by submitting our proposed XML
> additions if we are to avoid diving into the schemas. Our idea is to
> have the NFS generate XML based on issue 90, where you have a network
> disk, a source protocol, a host, and a new NFS tag which has a user
> attribute and a group attribute (both required). In terms of the rng
> schema, we would make it look similar to the VxHS schema
> (diskSourceNetworkProtocolVxHS) except that below the
> diskSourceNetworkHost we would also interweave a diskSourceNFS
> reference, which would require both user and group.
(since it's already quite late in the day on a Friday where Peter is
located, I'll make an attempt to answer for him :-)
I believe what he's asking for is just an email that says something like
(names and organizations completely fabricated on the spot for sake of
example):
"Our idea is to implement this new feature by adding a new value "blorg"
to the <bipple> element "blox", and an optional <bumble> subelement of
<bipple) that contains blahblahbobloblaw details, like this:
<device something='xyzzy'>
<bipple blox='blorg'>
<blorg blahblah='bobloblaw'>lawblog</blorg>
</bipple>
...
So, what do you think?"
or whatever. i.e., not a vague description or a formal RNG
representation of the changes you want to make, but short and specific
description along with an example of what those changes will look like
in an actual XML config document - something like the descriptions and
example XML bits in https://www.libvirt.org/formatdomain.html. This is
*much* quicker to parse and discuss than an RNG grammar :-)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201120/1a572830/attachment-0001.htm>
More information about the libvir-list
mailing list