Re: Issue 90 Further Clarifications

On 11/20/20 1:07 PM, Dustan B Helm wrote: On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa(a)redhat.com&gt; 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 :-)