RFC PATCH: Issue 90 (Test Clarification)
We addressed the feedback from our previous RFC patch for the most part. Under src/util/virstoragefile.c, we left a cast to an integer pointer that Peter mentioned because we were unable to provide a better solution. We've written some tests for our code but our testing environment is not working locally (meson doesn't even recognize the project as a meson build project) and so we can't regenerate output or test our tests. It's probably bad practice but the only solution we could think of that would allow us to check our tests was just to email you what we've got. Sorry if it's not up to standard, but please let us know if there's a better way to do it or if you spot any problems in these tests. Under tests/qemuxml2xmltest.c: DO_TEST_CAPS_LATEST("disk-network-nfs"); The same line would be in tests/qemuxml2argvtest.c We created the file tests/qemuxml2argvdata/disk-network-nfs.xml: <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory unit='KiB'>219136</memory> <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <disk type='network' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source protocol='nfs' name='/foo/bar/baz'> <host name='example.com' port='2049'/> <nfs user='nfs-user' group='nfs-group'/> </source> <target dev='vda' bus='virtio'/> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> <controller type='usb' index='0'/> <controller type='pci' index='0' model='pci-root'/> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <memballoon model='none'/> </devices> </domain> We have under tests/qemublocktest.c: TEST_JSON_FORMAT_NET(“<source protocol=’nfs’ name=’/foo/bar/baz’>\n” “ <host name=’example.com’ port=’2049’/>\n” “ <nfs user=’USER’ group=’GROUP’/>\n” "</source>\n”); and TEST_IMAGE_CREATE(“network-nfs-qcow2”, NULL); And finally under tests/virstoragetest.c: TEST_BACKING_PARSE(“json:{\”file\”:{\”driver\”:\”nfs\”,” “\”user\”:\”USER\”,” “\”group\”:\”GROUP\”,” “\”server\”: { \”host\”:\”example.com\”,” “\”port\”:\”2049\”” ”}” “}” “}”, “<source protocol=’nfs’ name=’/foo/bar/baz’>\n” “ <host name=’example.com’ port=’2049’/>\n” “ <nfs user=’USER’ group=’GROUP’/>\n” “</source>\n”); Again, sorry if this looks awful. Please let us know if there's a more practical way to do this because submitting a commit with these tests would guarantee that the tests fail and the commit wouldn't be mergeable due to our environment issues, or if you see anything wrong with these tests.
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/libxl/libxl_conf.c | 1 + src/libxl/xen_xl.c | 1 + src/qemu/qemu_block.c | 3 +++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 10 ++++++++++ src/qemu/qemu_snapshot.c | 3 +++ src/util/virstoragefile.c | 6 ++++++ src/util/virstoragefile.h | 1 + 8 files changed, 26 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 00748e21e8..6a8ae27f54 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -941,6 +941,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index ba0942601f..17b93d0f5c 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1600,6 +1600,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 4640e339c0..b224a550f3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1180,6 +1180,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, return NULL; break; + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportEnumRangeError(virStorageNetProtocol, src->protocol); @@ -2111,6 +2112,7 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: @@ -2502,6 +2504,7 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_ISCSI: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_HTTP: case VIR_STORAGE_NET_PROTOCOL_HTTPS: case VIR_STORAGE_NET_PROTOCOL_FTP: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b06a086e18..c58f39ebf1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1044,6 +1044,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, _("'ssh' protocol is not yet supported")); return NULL; + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bfb6e23942..692bc925c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4626,6 +4626,14 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src, return -1; } + /* NFS protocol may only be used if current QEMU supports blockdev */ + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->protocol == VIR_STORAGE_NET_PROTOCL_NFS && + !blockdev) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'nfs' protocol is not supported with this QEMU binary")); + } + return 0; } @@ -9630,6 +9638,8 @@ qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_FTP: case VIR_STORAGE_NET_PROTOCOL_FTPS: case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_NFS: + /* Assumed NFS doesn't support TLS (needs Kerberos) */ case VIR_STORAGE_NET_PROTOCOL_SSH: if (src->haveTLS == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 15494c3415..7e89a8839b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -413,6 +413,7 @@ qemuSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDefPtr snapdisk, case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " @@ -501,6 +502,7 @@ qemuSnapshotPrepareDiskExternalActive(virDomainObjPtr vm, case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external active snapshots are not supported on " @@ -631,6 +633,7 @@ qemuSnapshotPrepareDiskInternal(virDomainDiskDefPtr disk, case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("internal inactive snapshots are not supported on " diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index fac93118fd..5a57e5d12d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -90,6 +90,7 @@ VIR_ENUM_IMPL(virStorageNetProtocol, "tftp", "ssh", "vxhs", + "nfs", ); VIR_ENUM_IMPL(virStorageNetHostTransport, @@ -3152,6 +3153,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SSH: case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed backing store path for protocol %s"), protocol); @@ -4627,6 +4629,10 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) case VIR_STORAGE_NET_PROTOCOL_VXHS: return 9999; + case VIR_STORAGE_NET_PROTOCOL_NFS: + /* return based on exmaple and SUSE support docs */ + return 2049; + case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: return 0; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 87763cf389..c5d5f0233a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -135,6 +135,7 @@ typedef enum { VIR_STORAGE_NET_PROTOCOL_TFTP, VIR_STORAGE_NET_PROTOCOL_SSH, VIR_STORAGE_NET_PROTOCOL_VXHS, + VIR_STORAGE_NET_PROTOCOL_NFS, VIR_STORAGE_NET_PROTOCOL_LAST } virStorageNetProtocol; -- 2.29.0
On Wed, Dec 16, 2020 at 19:37:49 -0600, Ryan Gahagan wrote: Please add a commit message.
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/libxl/libxl_conf.c | 1 + src/libxl/xen_xl.c | 1 + src/qemu/qemu_block.c | 3 +++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 10 ++++++++++ src/qemu/qemu_snapshot.c | 3 +++ src/util/virstoragefile.c | 6 ++++++ src/util/virstoragefile.h | 1 + 8 files changed, 26 insertions(+)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bfb6e23942..692bc925c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4626,6 +4626,14 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src, return -1; }
+ /* NFS protocol may only be used if current QEMU supports blockdev */ + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->protocol == VIR_STORAGE_NET_PROTOCL_NFS && + !blockdev) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'nfs' protocol is not supported with this QEMU binary")); + }
This doesn't belong to this patch. This should be part of the patch implementing the qemu support for it. In this patch you are adding a new type and we try to minimize and separate changes.
+ return 0; }
[..]
@@ -4627,6 +4629,10 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) case VIR_STORAGE_NET_PROTOCOL_VXHS: return 9999;
+ case VIR_STORAGE_NET_PROTOCOL_NFS: + /* return based on exmaple and SUSE support docs */
It's better to add a link rather than a statement that is hard to verify.
+ return 2049; + case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: return 0;
We'll discuss the lack of tests separately. The rest of this patch looks good.
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/util/virstoragefile.c | 8 ++++++++ src/util/virstoragefile.h | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5a57e5d12d..cff6dabd9e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2446,6 +2446,11 @@ virStorageSourceCopy(const virStorageSource *src, def->ssh_host_key_check_disabled = src->ssh_host_key_check_disabled; def->ssh_user = g_strdup(src->ssh_user); + def->nfs_user = g_strdup(src->nfs_user); + def->nfs_group = g_strdup(src->nfs_group); + def->nfs_uid = src->nfs_uid; + def->nfs_gid = src->nfs_gid; + return g_steal_pointer(&def); } @@ -2686,6 +2691,9 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->ssh_user); + VIR_FREE(def->nfs_user); + VIR_FREE(def->nfs_group); + virStorageSourceInitiatorClear(&def->initiator); /* clear everything except the class header as the object APIs diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c5d5f0233a..64fc519f87 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -385,6 +385,11 @@ struct _virStorageSource { /* these must not be used apart from formatting the output JSON in the qemu driver */ char *ssh_user; bool ssh_host_key_check_disabled; + + char *nfs_user; + char *nfs_group; + uid_t nfs_uid; + gid_t nfs_gid; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); -- 2.29.0
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- docs/formatdomain.rst | 11 +++++++++- docs/schemas/domaincommon.rng | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..23a7bca643 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2370,6 +2370,14 @@ paravirtualized driver is specified via the ``disk`` element. </source> <target dev='sdb' bus='scsi'/> </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nfs' name='PATH'> + <host name='example.com' port='2049'/> + <nfs user='USER' group='GROUP'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> <disk type='network' device='lun'> <driver name='qemu' type='raw'/> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> @@ -2491,7 +2499,7 @@ paravirtualized driver is specified via the ``disk`` element. ``network`` The ``protocol`` attribute specifies the protocol to access to the requested image. Possible values are "nbd", "iscsi", "rbd", "sheepdog", - "gluster", "vxhs", "http", "https", "ftp", ftps", or "tftp". + "gluster", "vxhs", "nfs", "http", "https", "ftp", ftps", or "tftp". For any ``protocol`` other than ``nbd`` an additional attribute ``name`` is mandatory to specify which volume/image will be used. @@ -2601,6 +2609,7 @@ paravirtualized driver is specified via the ``disk`` element. sheepdog one of the sheepdog servers (default is localhost:7000) zero or one 7000 gluster a server running glusterd daemon one or more ( :since:`Since 2.1.0` ), just one prior to that 24007 vxhs a server running Veritas HyperScale daemon only one 9999 + nfs a server running Network File System only one ( :since:`Since 7.0.0` ) 2049 ======== ======================================================= ============================================================ ================ gluster supports "tcp", "rdma", "unix" as valid values for the transport diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 795b654feb..6b321b1ca3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1774,6 +1774,27 @@ </element> </define> + <define name="diskSourceNetworkNFS"> + <element name="nfs"> + <optional> + <attribute name="user"> + <choice> + <ref name="unsignedInt"/> + <ref name="genericName"/> + </choice> + </attribute> + </optional> + <optional> + <attribute name="group"> + <choice> + <ref name="unsignedInt"/> + <ref name="genericName"/> + </choice> + </attribute> + </optional> + </element> + </define> + <define name="diskSourceNetworkProtocolRBD"> <element name="source"> <interleave> @@ -2039,6 +2060,22 @@ </element> </define> + <define name="diskSourceNetworkProtocolNFS"> + <element name="source"> + <interleave> + <attribute name="protocol"> + <choice> + <value>nfs</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceCommon"/> + <ref name="diskSourceNetworkHost"/> + <ref name="diskSourceNetworkNFS"/> + </interleave> + </element> + </define> + <define name="diskSourceNetwork"> <attribute name="type"> <value>network</value> @@ -2053,6 +2090,7 @@ <ref name="diskSourceNetworkProtocolFTPS"/> <ref name="diskSourceNetworkProtocolSimple"/> <ref name="diskSourceNetworkProtocolVxHS"/> + <ref name="diskSourceNetworkProtocolNFS"/> </choice> </define> -- 2.29.0
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b301ac0a08..565ca680c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6888,6 +6888,23 @@ virDomainStorageNetworkParseHosts(xmlNodePtr node, } +static void +virDomainStorageNetworkParseNFS(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src) +{ + xmlNodePtr nfsnode = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt); + + ctxt->node = node; + + if ((nfsnode = virXPathNode("./nfs", ctxt))) { + src->nfs_user = virXMLPropString(nfsnode, "user"); + src->nfs_group = virXMLPropString(nfsnode, "group"); + } +} + + static int virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, xmlXPathContextPtr ctxt, @@ -8252,6 +8269,9 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, if (virDomainStorageNetworkParseHosts(node, ctxt, &src->hosts, &src->nhosts) < 0) return -1; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS) + virDomainStorageNetworkParseNFS(node, ctxt, src); + virStorageSourceNetworkAssignDefaultPorts(src); virStorageSourceInitiatorParseXML(ctxt, &src->initiator); @@ -23805,6 +23825,19 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf, virBufferAddLit(childBuf, "/>\n"); } + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS && + (src->nfs_user || src->nfs_group)) { + virBufferAddLit(childBuf, "<nfs"); + + if (src->nfs_user) + virBufferEscapeString(childBuf, " user='%s'", src->nfs_user); + if (src->nfs_group) + virBufferEscapeString(childBuf, " group='%s'", src->nfs_group); + + virBufferAddLit(childBuf, "/>\n"); + } + + virBufferEscapeString(childBuf, "<snapshot name='%s'/>\n", src->snapshot); virBufferEscapeString(childBuf, "<config file='%s'/>\n", src->configFile); -- 2.29.0
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/qemu/qemu_block.c | 48 +++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b224a550f3..c0f47eedc0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -674,6 +674,42 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src, } +static virJSONValuePtr +qemuBlockStorageSourceGetNFSProps(virStorageSourcePtr src) +{ + g_autoptr(virJSONValue) server = NULL; + virJSONValuePtr ret = NULL; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NFS protocol accepts only one host")); + return NULL; + } + + if (!(server = qemuBlockStorageSourceBuildJSONInetSocketAddress(&src->hosts[0]))) + return NULL; + + /* NFS disk specification example: + * { driver:"nfs", + * user: "0", + * group: "0", + * server: {type:"tcp", host:"1.2.3.4", port:9999}} + */ + ignore_value(virJSONValueObjectCreate(&ret, + "a:server", &server, NULL)); + + if (src->nfs_uid != -1 && + virJSONValueObjectAdd(ret, "i:user", src->nfs_uid, NULL) < 0) + return NULL; + + if (src->nfs_gid != -1 && + virJSONValueObjectAdd(ret, "i:group", src->nfs_gid, NULL) < 0) + return NULL; + + return ret; +} + + static virJSONValuePtr qemuBlockStorageSourceGetCURLProps(virStorageSourcePtr src, bool onlytarget) @@ -1181,6 +1217,11 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, break; case VIR_STORAGE_NET_PROTOCOL_NFS: + driver = "nfs"; + if (!(fileprops = qemuBlockStorageSourceGetNFSProps(src))) + return NULL; + break; + case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportEnumRangeError(virStorageNetProtocol, src->protocol); @@ -2500,11 +2541,16 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSourcePtr src, return -1; break; + case VIR_STORAGE_NET_PROTOCOL_NFS: + driver = "nfs"; + if (!(location = qemuBlockStorageSourceGetNFSProps(src))) + return -1; + break; + /* unsupported/impossible */ case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_ISCSI: case VIR_STORAGE_NET_PROTOCOL_VXHS: - case VIR_STORAGE_NET_PROTOCOL_NFS: case VIR_STORAGE_NET_PROTOCOL_HTTP: case VIR_STORAGE_NET_PROTOCOL_HTTPS: case VIR_STORAGE_NET_PROTOCOL_FTP: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 692bc925c6..65c751e1d9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9598,6 +9598,42 @@ qemuProcessPrepareStorageSourceTLSNBD(virStorageSourcePtr src, } +/* qemuPrepareStorageSourceNFS: + * @src: source for a disk + * + * If src is an NFS source, translate nfs_user and nfs_group + * into a uid and gid field. If these strings are empty (ie "") + * then provide the hypervisor default uid and gid. + */ + static int + qemuDomainPrepareStorageSourceNFS(virStorageSourcePtr src) + { + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) + return 0; + + if ((virStorageNetProtocol) src->protocol != VIR_STORAGE_NET_PROTOCOL_NFS) + return 0; + + if (src->nfs_user) { + if (virGetUserID(src->nfs_user, &src->nfs_uid) < 0) + return -1; + } else { + /* -1 indicates default UID */ + src->nfs_uid = -1; + } + + if (src->nfs_group) { + if (virGetGroupID(src->nfs_group, &src->nfs_gid) < 0) + return -1; + } else { + /* -1 indicates default GID */ + src->nfs_gid = -1; + } + + return 0; +} + + /* qemuProcessPrepareStorageSourceTLS: * @source: source for a disk * @cfg: driver configuration @@ -10409,6 +10445,9 @@ qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDefPtr disk, priv) < 0) return -1; + if (qemuDomainPrepareStorageSourceNFS(src) < 0) + return -1; + return 0; } -- 2.29.0
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/util/virstoragefile.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cff6dabd9e..341eac2eb7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3805,6 +3805,42 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, } +static int +virStorageSourceParseBackingJSONNFS(virStorageSourcePtr src, + virJSONValuePtr json, + const char *jsonstr G_GNUC_UNUSED, + int opaque G_GNUC_UNUSED) +{ + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + int gotUID = virJSONValueObjectGetNumberInt(json, "user", (int *)(&src->nfs_uid)); + int gotGID = virJSONValueObjectGetNumberInt(json, "group", (int *)(&src->nfs_gid)); + + if (!server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'server' attribute in JSON backing definition for NFS volume")); + return -1; + } + + if (gotUID < 0 || gotGID < 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'user' or 'group' attribute in JSON backing definition for NFS volume")); + return -1; + } + + src->type = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_NFS; + + src->hosts = g_new0(virStorageNetHostDef, 1); + src->nhosts = 1; + + if (virStorageSourceParseBackingJSONInetSocketAddress(src->hosts, + server) < 0) + return -1; + + return 0; +} + + static int virStorageSourceParseBackingJSONNVMe(virStorageSourcePtr src, virJSONValuePtr json, @@ -3864,6 +3900,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"ssh", false, virStorageSourceParseBackingJSONSSH, 0}, {"rbd", false, virStorageSourceParseBackingJSONRBD, 0}, {"raw", true, virStorageSourceParseBackingJSONRaw, 0}, + {"nfs", false, virStorageSourceParseBackingJSONNFS, 0}, {"vxhs", false, virStorageSourceParseBackingJSONVxHS, 0}, {"nvme", false, virStorageSourceParseBackingJSONNVMe, 0}, }; -- 2.29.0
On Wed, Dec 16, 2020 at 19:37:48 -0600, Ryan Gahagan wrote:
We addressed the feedback from our previous RFC patch for the most part. Under src/util/virstoragefile.c, we left a cast to an integer pointer that Peter mentioned because we were unable to provide a better
Casting uid_t * to int * will work in this instance but is not acceptable. The problem with casting pointers is that if sizeof(uid_t) != sizeof(int) casting a pointer could result into accessing invalid memory, while if you just assign the value of integers of distinct sizes you get an overflow at worst. That must be changed in the code.
solution. We've written some tests for our code but our testing environment is not working locally (meson doesn't even recognize the project as a meson build project) and so we can't regenerate output or test our tests.
Could you elaborate? As in, post exact steps and output what you've done and what's broken.
It's probably bad practice but the only solution we could think of that would allow us to check our tests was just to email you what we've got. Sorry if it's not up to standard, but please let us know if there's a better way to do it or if you spot any problems in these tests.
It's better to commit what you have and ask directly, ideally including output you are (not) getting.
Under tests/qemuxml2xmltest.c: DO_TEST_CAPS_LATEST("disk-network-nfs");
The same line would be in tests/qemuxml2argvtest.c
We created the file tests/qemuxml2argvdata/disk-network-nfs.xml: <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory unit='KiB'>219136</memory> <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <disk type='network' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source protocol='nfs' name='/foo/bar/baz'> <host name='example.com' port='2049'/> <nfs user='nfs-user' group='nfs-group'/> </source> <target dev='vda' bus='virtio'/> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> <controller type='usb' index='0'/> <controller type='pci' index='0' model='pci-root'/> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <memballoon model='none'/> </devices> </domain>
We have under tests/qemublocktest.c: TEST_JSON_FORMAT_NET(“<source protocol=’nfs’ name=’/foo/bar/baz’>\n” “ <host name=’example.com’ port=’2049’/>\n” “ <nfs user=’USER’ group=’GROUP’/>\n” "</source>\n”); and TEST_IMAGE_CREATE(“network-nfs-qcow2”, NULL);
And finally under tests/virstoragetest.c: TEST_BACKING_PARSE(“json:{\”file\”:{\”driver\”:\”nfs\”,” “\”user\”:\”USER\”,” “\”group\”:\”GROUP\”,” “\”server\”: { \”host\”:\”example.com\”,” “\”port\”:\”2049\”” ”}” “}” “}”, “<source protocol=’nfs’ name=’/foo/bar/baz’>\n” “ <host name=’example.com’ port=’2049’/>\n” “ <nfs user=’USER’ group=’GROUP’/>\n” “</source>\n”);
Again, sorry if this looks awful. Please let us know if there's a more practical way to do this because submitting a commit with these tests would guarantee that the tests fail and the commit wouldn't be mergeable due to our environment issues, or if you see anything wrong with these tests.
Note that patches without tests are not acceptable either. Additionally I'll not be copying your changes to appropriate places. Please add the test code to appropriate places. Also note that the upstream test-suite run in the CI does actually provide the expected output. Obviously you can't use the ENV variable to automatically overwrite your files, but you certainly can copy out the diffs from the CI. Just commit empty files in place for the output files and the CI will complain that they differ including the full difference. If you don't post what's wrong I will not be guessing, so if you want any input from us, be sure to provide as much information as you have including exact steps.
On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa <pkrempa@redhat.com> wrote:
Also note that the upstream test-suite run in the CI does actually provide the expected output. Obviously you can't use the ENV variable to automatically overwrite your files, but you certainly can copy out the diffs from the CI. Just commit empty files in place for the output files and the CI will complain that they differ including the full difference.
We tried doing this and we failed on CI, but the pipeline jobs didn't output a diff, and instead simply errored out with the message "Full log written to /builds/bschoney/libvirt/build/meson-private/dist-build/meson-logs/testlog.txt". How can we actually view the diff against our blank file? We're trying to find the output for the qemuxml2argvdata args file for our new test but it's not being printed anywhere.
On Fri, Dec 18, 2020 at 15:14:01 -0600, Ryan Gahagan wrote:
On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa <pkrempa@redhat.com> wrote:
Also note that the upstream test-suite run in the CI does actually provide the expected output. Obviously you can't use the ENV variable to automatically overwrite your files, but you certainly can copy out the diffs from the CI. Just commit empty files in place for the output files and the CI will complain that they differ including the full difference.
We tried doing this and we failed on CI, but the pipeline jobs didn't output a diff, and instead simply errored out with the message "Full log written to /builds/bschoney/libvirt/build/meson-private/dist-build/meson-logs/testlog.txt". How can we actually view the diff against our blank file? We're trying to find the output for the qemuxml2argvdata args file for our new test but it's not being printed anywhere.
You can actually see some of the test output in your CI pipeline runs: https://gitlab.com/bschoney/libvirt/-/jobs/922060289#L2961 anyways, it's not worth for me to debug why some of the output might be skipped (perhaps missing empty output files). Please make sure your local env is able to build libvirt, so that you can use VIR_TEST_REGENERATE_OUTPUT. That is certainly the simplest way. In addition you certainly want to test your changes in action, and our CI AFAIK doesn't provide built packages.
We were able to get the testing environment running after some trial-and-error. The issue was that the version of libtirpc was out of date, causing an error in meson.build when the XDR dependency was being read. By updating the libtirpc-dev package we were able to start building locally. However, we did have some questions about our tests. For now we wanted to focus specifically on the qemublocktest.c file. We have the function virDomainDiskSourceNetworkParse in src/conf/domain_conf.c where XML is parsed into virStorageSource data and the function qemuBlockStorageSourceCreateGetStorageProps under src/qemu/qemu_block.c where this data is converted into a JSON object. Our understanding of the tests/qemublocktest.c file is that we provide an XML string example via TEST_JSON_FORMAT_NET and that this string was parsed into JSON then formatted back into XML to make sure the process produced the same output as the input. One issue is that for some reason our user and group strings (i.e. “<nfs user=’+1’ group=’+2’/>”) are not getting parsed into integers. The nfs_uid and nfs_gid fields are never set and therefore result in a 0 every time. We have code to fix this in a method under qemuDomainPrepareStorageSourceBlockdev in the file src/qemu/qemu_domain.c which looks up the user and group and stores the values we expect, but this method isn’t getting called. Where in this process have we missed something? Should we move the virGetUserID and virGetGroupID method calls to somewhere else? We were also curious about the methods which parse the JSON values back into data. In our case, this is virStorageSourceParseBackingJSONNFS in src/util/virstoragefile.c. This method tries to retrieve the user and group integers from the NFS JSON object. However, when constructing the JSON object in the getStorageProps method we don't actually add those values if they're "default" values. In JSON terms this means that the integers are undefined, but how does C interpret it? Does the retrieve method return 0 or simply fail?
On Mon, Dec 21, 2020 at 17:10:35 -0600, Ryan Gahagan wrote:
We were able to get the testing environment running after some trial-and-error. The issue was that the version of libtirpc was out of date, causing an error in meson.build when the XDR dependency was being read. By updating the libtirpc-dev package we were able to start building locally. However, we did have some questions about our tests. For now we wanted to focus specifically on the qemublocktest.c file.
We have the function virDomainDiskSourceNetworkParse in src/conf/domain_conf.c where XML is parsed into virStorageSource data and the function qemuBlockStorageSourceCreateGetStorageProps under src/qemu/qemu_block.c where this data is converted into a JSON object. Our understanding of the tests/qemublocktest.c file is that we provide an XML string example via TEST_JSON_FORMAT_NET and that this string was parsed into JSON then formatted back into XML to make sure the process produced the same output as the input. One issue is that for some reason our user and group strings (i.e. “<nfs user=’+1’ group=’+2’/>”) are not getting parsed into integers. The nfs_uid and nfs_gid fields are never set and therefore result in a 0 every time. We have code to fix this in a method under qemuDomainPrepareStorageSourceBlockdev in the file src/qemu/qemu_domain.c which looks up the user and group and stores the values we expect, but this method isn’t getting called. Where in this process have we missed something? Should we move the virGetUserID and virGetGroupID method calls to somewhere else?
No. The issue is that the tests shouldn't really be allowed to look up the UID/GID in the system. For that reason the test code doesn't actually call qemuDomainPrepareStorageSourceBlockdev. For this test, you'll be better off adding a hack to fill in the uid/gid values (e.g. by checking that they start with a + to be safe.
We were also curious about the methods which parse the JSON values back into data. In our case, this is virStorageSourceParseBackingJSONNFS in src/util/virstoragefile.c. This method tries to retrieve the user and group integers from the NFS JSON object. However, when constructing the JSON object in the getStorageProps method we don't actually add those values if they're "default" values. In JSON terms this means that the integers are undefined, but how does C interpret it? Does the retrieve method return 0 or simply fail?
The formatter omits the values if they are default. According to the QMP schema they can be missing. The parser thus must cope when the JSON object doesn't have the values populated.
On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa <pkrempa@redhat.com> wrote:
For this test, you'll be better off adding a hack to fill in the uid/gid values (e.g. by checking that they start with a + to be safe.
Our interpretation of what you mean here is to add a check in our parse method (specifically in virDomainStorageNetworkParseNFS, which is called by virDomainDIskSourceNetworkParse in domain_conf.c) which looks at the user/group strings in the XML and checks if their first character is a '+'. If so, assume that this string is an integer and not a name, then parse it into the nfs_uid or gid values. This would allow our tests to understand a property like "user='+2'" without having to call the preparseStorageSource methods. This functionality is built into virGetUserID, but because we can't call it we would have to duplicate the code. Is this an acceptable approach?
The formatter omits the values if they are default. According to the QMP schema they can be missing. The parser thus must cope when the JSON object doesn't have the values populated.
So will a method like "virJSONValueObjectGetNumberInt" return an error code (i.e. < 0) if the property is missing? If so, we could use this error code as an indicator that the property (e.g. user) should be the default value (e.g. -1). We also had some questions in regards to other tests. We won't submit it as an RFC patch because it isn't ready, but if you'd like to see our current code for reference you can view it on this branch <https://gitlab.com/bschoney/libvirt/-/tree/issue-90-rt2>. We're failing virschematest for our file disk-network-nfs.xml. The error is: "Extra element devices in interleave Element domain failed to validate content" This seems like there's a problem with our XML not matching the RNG docs. However, the problem appears to be with our <devices> tag, which was based on the XML in disk-backing-chains.xml. Why does our file report an error for this, but disk-backing-chains.xml does not? Also, both xml2argv and xml2xml tests are failing, even with the VIR_REGENERATE_OUTPUT environment variable set to 1. The error is: "libvirt: Domain Config error : missing source information for device vda" To be completely honest, we don't know what about our XML or schema is causing this error and so we aren't sure what the fix is. We've based our XML mostly on disk-network-vxhs.xml and disk-backing-chains.xml, but don't understand why those pass and ours doesn't.
On Wed, Dec 23, 2020 at 14:28:04 -0600, Ryan Gahagan wrote:
On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa <pkrempa@redhat.com> wrote:
For this test, you'll be better off adding a hack to fill in the uid/gid values (e.g. by checking that they start with a + to be safe.
Our interpretation of what you mean here is to add a check in our parse method (specifically in virDomainStorageNetworkParseNFS, which is called by virDomainDIskSourceNetworkParse in domain_conf.c) which looks at the user/group strings in the XML and checks if their first character is a '+'. If so, assume that this string is an integer and not a name, then parse it into the nfs_uid or gid values. This would allow our tests to understand a property like "user='+2'" without having to call the preparseStorageSource methods. This functionality is built into virGetUserID, but because we can't call it we would have to duplicate the code. Is this an acceptable approach?
TEST_JSON_FORMAT_NET does an XML->JSON->XML coversion to see whether the back-and-forth conversion works, but that's not really entirely necessary. The test was added so that the JSON bit can be validated, but meanwhile we do that also in the qemuxml2argvtest. You can instead try a simpler method. Add a TEST_BACKING_PARSE case into tests/virstoragetest.c That tests just JSON->XML so has fewer moving parts. Since the qemuxml2argv test case is mandatory the second part will be covered regardless.
The formatter omits the values if they are default. According to the QMP schema they can be missing. The parser thus must cope when the JSON object doesn't have the values populated.
So will a method like "virJSONValueObjectGetNumberInt" return an error code (i.e. < 0) if the property is missing? If so, we could use this error code as an indicator that the property (e.g. user) should be the default value (e.g. -1).
We also had some questions in regards to other tests. We won't submit it as an RFC patch because it isn't ready, but if you'd like to see our current code for reference you can view it on this branch <https://gitlab.com/bschoney/libvirt/-/tree/issue-90-rt2>.
We're failing virschematest for our file disk-network-nfs.xml. The error is: "Extra element devices in interleave Element domain failed to validate content"
You can use the 'jing' validator for a more expressive output: $ jing /home/pipo/libvirt/docs/schemas/domain.rng /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:29:55: error: attribute "protocol" not allowed here; expected attribute "file", "index" or "startupPolicy" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:29:55: error: attribute "name" not allowed here; expected attribute "file", "index" or "startupPolicy" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:30:56: error: element "host" not allowed here; expected the element end-tag or element "encryption", "seclabel" or "slices" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:33:29: error: value of attribute "type" is invalid; must be equal to "bochs", "cloop", "cow", "dir", "dmg", "fat", "iso", "luks", "ploop", "qcow", "qcow2", "qed", "raw", "vdi", "vhd", "vmdk" or "vpc"
This seems like there's a problem with our XML not matching the RNG docs. However, the problem appears to be with our <devices> tag, which was based on the XML in disk-backing-chains.xml. Why does our file report an error for this, but disk-backing-chains.xml does not?
The problem is that the definition of the second disk in 'disk-network-nfs' is plainly broken: <disk> <driver name='qemu' type='qcow2'/> <source protocol='gluster' name='Volume2/Image'> <host transport='unix' socket='/path/to/sock'/> </source> <backingStore type='network' index='1'> <format type='nfs'/> <source protocol='nfs' name='/backing/store/nfs'> <host name='example.org'/> <nfs user='USER' group='GROUP'/> </source> <backingStore/> </backingStore> <target dev='vda' bus='virtio'/> </disk> Firstly 'type' and 'device' are mandatory attributes for the <disk> element. Also you've got two disks with <target dev='vda'> but the target must be unique. Also <format type='nfs'> is wrong. The format is raw/qcow2/etc After fixing all of those I'm getting 184) QEMU XML-2-ARGV disk-network-nfs.x86_64-latest ... libvirt: error : invalid argument: Failed to parse user 'USER' ALL of the above was possible to debug even without the XML schema validator though just by looking at the error message and finding where/why it's reported. Finally after fixing all of those with the following diff: diff --git a/tests/qemuxml2argvdata/disk-network-nfs.xml b/tests/qemuxml2argvdata/disk-network-nfs.xml index 8ff6859f1f..67d2843e01 100644 --- a/tests/qemuxml2argvdata/disk-network-nfs.xml +++ b/tests/qemuxml2argvdata/disk-network-nfs.xml @@ -18,26 +18,26 @@ <driver name='qemu' type='raw' cache='none'/> <source protocol='nfs' name='/foo/bar/baz'> <host name='example.com' port='2049'/> - <nfs user='nfs-user' group='nfs-group'/> + <nfs user='+6234' group='+12354'/> </source> <target dev='vda' bus='virtio'/> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> - <disk> + <disk type='network' device='disk'> <driver name='qemu' type='qcow2'/> <source protocol='gluster' name='Volume2/Image'> <host transport='unix' socket='/path/to/sock'/> </source> <backingStore type='network' index='1'> - <format type='nfs'/> + <format type='qcow2'/> <source protocol='nfs' name='/backing/store/nfs'> <host name='example.org'/> - <nfs user='USER' group='GROUP'/> + <nfs user='+1234' group='+5678'/> </source> <backingStore/> </backingStore> - <target dev='vda' bus='virtio'/> + <target dev='vdb' bus='virtio'/> </disk> <controller type='usb' index='0'/> <controller type='pci' index='0' model='pci-root'/> I get a QMP schema validation error: 184) QEMU XML-2-ARGV disk-network-nfs.x86_64-latest ... failed to validate -blockdev '{"driver":"nfs","server":{"host":"example.com","port":"2049"},"path":"/foo/bar/baz","user":6234,"group":12354,"node-name":"libvirt-3-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}' against QAPI schema: { driver: 'nfs' OK server: { host: 'example.com': OK port: ERROR: attribute not in schema This means that the JSON definition does not conform to the QMP schema provided by qemu. Looking at the QMP schema, you used wrong generator for the 'server' portion: The QMP schema in qemu.git is written as: { 'struct': 'BlockdevOptionsNfs', 'data': { 'server': 'NFSServer', 'path': 'str', '*user': 'int', '*group': 'int', '*tcp-syn-count': 'int', '*readahead-size': 'int', '*page-cache-size': 'int', '*debug': 'int' } } { 'struct': 'NFSServer', 'data': { 'type': 'NFSTransport', 'host': 'str' } } { 'enum': 'NFSTransport', 'data': [ 'inet' ] } It seems that you've picked qemuBlockStorageSourceBuildJSONInetSocketAddress which formats 'SocketAddress'-compatible JSON, but the QMP schema for NFS has it's own type. Thus only TCP is supoprted and 'port' is not configurable. This needs to be expressed and checked in libvirt too so that users don't try to configure the port or unix transport while it's impossible. (This also shows why qemuxml2argvtest is so important, as it does the validation against qemu. I'm also fairly certain that qemu would reject such config, so please make sure to test your code before submitting it)
Also, both xml2argv and xml2xml tests are failing, even with the VIR_REGENERATE_OUTPUT environment variable set to 1. The error is: "libvirt: Domain Config error : missing source information for device vda" To be completely honest, we don't know what about our XML or schema is causing this error and so we aren't sure what the fix is. We've based our XML mostly on disk-network-vxhs.xml and disk-backing-chains.xml, but don't understand why those pass and ours doesn't.
Because your XML is broken. If we can't parse it there's nothing to generate.
We've implemented all the changes you suggested and we've come across a slight wrinkle. We fixed our xml2argvdata test and we were working on the qemuxml2xmltest and found two tests which appear to have contradictory outputs. Specifically, we ran VIR_TEST_REGENERATE_OUTPUT=1 ./build/tests/qemuxml2xmltest and it generated some errors within VIR_TEST_RANGE=181-182. Inspecting these tests, we found what we believe to be a contradiction (which we aren't sure how to fix). Our output file (qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml) has a backingStore on line 35. Test 181 generates an output where this backing store does not have an index, but Test 182 generates an output where this backing store has an index='1' attribute. Using REGENERATE_OUTPUT command seems to satisfy test 182 but fails test 181. If we fix the XML to satisfy test 181, then test 182 will fail. We've attached some screenshots for clarity, if this sounds confusing. Why is this error arising, and how do we fix it?
On Mon, Dec 28, 2020 at 15:49:17 -0600, Ryan Gahagan wrote:
Our output file (qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml) has a backingStore on line 35. Test 181 generates an output where this backing store does not have an index, but Test 182 generates an output where this backing store has an index='1' attribute. Using REGENERATE_OUTPUT command seems to satisfy test 182 but fails test 181. If we fix the XML to satisfy test 181, then test 182 will fail. We've attached some screenshots for clarity, if this sounds confusing.
Please don't post screenshots of text. It's pointlessly bloating the mail and I can't respond inline. Please re-post all the relevant bits as text.
On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa <pkrempa@redhat.com> wrote:
Please don't post screenshots of text. It's pointlessly bloating the mail and I can't respond inline. Please re-post all the relevant bits as text.
Sorry about that, I didn't know what the policy on images was. Basically, we used the REGENERATE_OUTPUT for the xml2xmltest and inside the generated output there is a line which reads "<backingStore type='network' index='1'>". However, the xml2xmltest fails on test 181 with the error: Expect [ index='1'>] Actual [>] This backingStore is the only place in the file with an index='1', so logically we tried removing that attribute. However, this causes test 182 to fail (even though it passed before this change) with the error: Expect [>] Actual [ index='1'>] It seems like these tests are at odds with one another and we aren't sure what the cause or fix would be. If we use an index, then 181 fails, and if we don't, then 182 will fail. Do you know what might be causing this?
On Tue, Dec 29, 2020 at 10:17:31 -0600, Ryan Gahagan wrote:
On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa <pkrempa@redhat.com> wrote:
Please don't post screenshots of text. It's pointlessly bloating the mail and I can't respond inline. Please re-post all the relevant bits as text.
I've remembered a few other reasons to avoid screenshots. Mostly I can't copy&paste if I wanted to look for something you've posted and also the contents of the image are not indexed by search engines. Also please note that the reviews may seem nitpicky, but I'm trying to be thorough. By merging your code we'll need to support and maintain the code in the future even in cases when you decide to no longer participate in our project, thus we need to avoid as many future problems as possible.
Sorry about that, I didn't know what the policy on images was. Basically, we used the REGENERATE_OUTPUT for the xml2xmltest and inside the generated output there is a line which reads "<backingStore type='network' index='1'>". However, the xml2xmltest fails on test 181 with the error: Expect [ index='1'>] Actual [>]
This backingStore is the only place in the file with an index='1', so logically we tried removing that attribute. However, this causes test 182 to fail (even though it passed before this change) with the error: Expect [>] Actual [ index='1'>]
It seems like these tests are at odds with one another and we aren't sure what the cause or fix would be. If we use an index, then 181 fails, and if we don't, then 182 will fail. Do you know what might be causing this?
Ah, yes. That test is a bit magic when the definition XML of a live VM differs from a definition XML of an inactive VM and in such case VIR_TEST_REGENRATE_OUTPUT=1 will not do the right thing fully automatically. You need to create tests/qemuxml2xmloutdata/$TESTNAME-inactive.xml as an empty file and then re-REGENERATE the output. The presence of the -inactive file is registered by the test runner and then two distinct versions are allowed. Also note that if you provided full text output of the test I would be able to replace $TESTNAME by the actual file name you need to use, by copying and pasting. Also, in this case it wasn't necessary, but always provide at least link to your repository if you've modified the code. Making reviewers job as easy as possible is the best way to expedite the review process. Review is a rather scarse resource in many projects, so having the reviwer guess or look for additional data wastes the available resource.
participants (3)
-
Peter Krempa -
Ryan Gahagan -
Ryan Gahagan