7:37 p.m.
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.
7:37 p.m.
New subject: [RFC PATCH 1/6] conf: Add NFS disk protocol
Signed-off-by: Ryan Gahagan <rgahagan(a)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
4:22 a.m.
New subject: [RFC PATCH 1/6] conf: Add NFS disk protocol
On Wed, Dec 16, 2020 at 19:37:49 -0600, Ryan Gahagan wrote:
Please add a commit message.
Signed-off-by: Ryan Gahagan <rgahagan(a)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.
7:37 p.m.
New subject: [RFC PATCH 2/6] util: Added nfs params to virStorageSource
Signed-off-by: Ryan Gahagan <rgahagan(a)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
7:37 p.m.
New subject: [RFC PATCH 3/6] docs: added rng schema and formatdomain for NFS
Signed-off-by: Ryan Gahagan <rgahagan(a)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
7:37 p.m.
New subject: [RFC PATCH 4/6] conf: Added NFS XML format/parse methods
Signed-off-by: Ryan Gahagan <rgahagan(a)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
7:37 p.m.
New subject: [RFC PATCH 5/6] qemu: Added NFS JSON props methods
Signed-off-by: Ryan Gahagan <rgahagan(a)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
7:37 p.m.
New subject: [RFC PATCH 6/6] util: Added a backing store NFS parser
Signed-off-by: Ryan Gahagan <rgahagan(a)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
8 a.m.
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.
3:14 p.m.
On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa <pkrempa(a)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.
5:03 a.m.
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(a)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.
5:10 p.m.
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?
11:45 a.m.
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.
2:28 p.m.
On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa <pkrempa(a)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.
6:09 a.m.
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(a)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.
3:49 p.m.
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?
10:11 a.m.
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.
10:17 a.m.
On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa <pkrempa(a)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?
10:39 a.m.
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(a)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.
1616
days inactive
1628
days old
18 comments
3 participants
participants (3)
-
Peter Krempa -
Ryan Gahagan -
Ryan Gahagan