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.
