On 11/20/20 1:07 PM, Dustan B Helm wrote:

On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa@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>
> > Dustan Helm <https://gitlab.com/dustan.helm> @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#>
> >
> > 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 :-)