RFC PATCH: Issue 90
This is a request for comments on our patch for Issue 90. We didn't add in the test yet because we've had a ton of trouble getting the testing suite to work with our new test files, so they've omitted for now. We also did not add any logic to the attach-disk method yet because we wanted to focus on this set of changes first.
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 | 2 ++ src/qemu/qemu_snapshot.c | 3 +++ src/util/virstoragefile.c | 6 ++++++ src/util/virstoragefile.h | 1 + 8 files changed, 18 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 766f76020c..b5a4a22ed3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9629,6 +9629,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 Fri, Dec 11, 2020 at 4:01 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> ---
The patch for compatibilities usually is on src/qemu/qemu_capabilities* and tests/qemucapabilitiesdata/caps_*. Because nfs disk is supported since qemu v2.9( https://github.com/qemu/qemu/blob/master/qapi/block-core.json#L3759), the compatibilities flags should be added to check whether the target qemu supports nfs disk. https://gitlab.com/libvirt/libvirt/-/commit/9825f71b53014bd2b418680ffeb2b27d... is a example: The diff in src/qemu/qemu_capabilities.* defines the name & flag of this capability flag, and the method to query the capability. For more information of the capability query, you can refer to the code comments of virQEMUQAPISchemaPathGet in libvirt and the query-qmp-schema in qemu https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html The diff in tests/qemucapabilitiesdata/caps_*.xml defines the capability unit tests for different arhes and qemu versions. For this patch, the diff in src/util/* and src/libxl/* could be included in the "util: ..." patch. The diff of src/qemu/* is better to be included in the "qemu: ..." patch.
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 | 2 ++ src/qemu/qemu_snapshot.c | 3 +++ src/util/virstoragefile.c | 6 ++++++ src/util/virstoragefile.h | 1 + 8 files changed, 18 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 766f76020c..b5a4a22ed3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9629,6 +9629,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 Thu, Dec 10, 2020 at 14:00:02 -0600, Ryan Gahagan wrote: In subject/summary. We don't have anything which we'd prefix with 'compatibilities:'. A better summary will be: conf: Add NFS disk protocol
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 | 2 ++ src/qemu/qemu_snapshot.c | 3 +++ src/util/virstoragefile.c | 6 ++++++ src/util/virstoragefile.h | 1 + 8 files changed, 18 insertions(+)
On Fri, Dec 11, 2020 at 3:35 AM Peter Krempa <pkrempa@redhat.com> wrote:
In subject/summary. We don't have anything which we'd prefix with 'compatibilities:'.
Just to confirm, does this mean that we should not implement the feedback Han Han suggested about the NFS capability flags and instead leave the commit as-is (except for the summary)? We didn't provide an NFS CAPS flag because in a previous email you had suggested: "- there's no need to add a specific capability for the NFS protocol as it predates libvirt's use of -blockdev (QEMU_CAPS_BLOCKDEV). You have to add a check for it to qemuDomainValidateStorageSource based on the above capabapility." We will provide this check in qemuDomainValidateStorageSource, but do we need to worry about CAPS flags?
(answering for Peter since he is on "vacation", so it may be awhile before he gets around to this) On 12/14/20 5:54 PM, Ryan Gahagan wrote:
On Fri, Dec 11, 2020 at 3:35 AM Peter Krempa <pkrempa@redhat.com <mailto:pkrempa@redhat.com>> wrote:
In subject/summary. We don't have anything which we'd prefix with 'compatibilities:'.
Just to confirm, does this mean that we should not implement the feedback Han Han suggested about the NFS capability flags and instead leave the commit as-is (except for the summary)?
Yes. What Han Han was discussing is the QEMU capabilities flags, which indicate which of a large set of features are supported by a particular QEMU binary. Since, as you say below, any QEMU binary that is capable of using blockdev is by definition capable of using NFS, there is no need to add an extra flag for NFS.
We didn't provide an NFS CAPS flag because in a previous email you had suggested: "- there's no need to add a specific capability for the NFS protocol as it predates libvirt's use of -blockdev (QEMU_CAPS_BLOCKDEV). You have to add a check for it to qemuDomainValidateStorageSource based on the above capabapility." We will provide this check in qemuDomainValidateStorageSource, but do we need to worry about CAPS flags?
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
On Fri, Dec 11, 2020 at 4:01 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
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;
Only one pair of (nfs_user,nfs_group) or (nfs_uid,nfs_gid) is enough to identify the nfs connection. Please remove a duplicated pair.
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); -- 2.29.0
On Fri, Dec 11, 2020 at 12:19:10 +0800, Han Han wrote:
On Fri, Dec 11, 2020 at 4:01 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
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;
Only one pair of (nfs_user,nfs_group) or (nfs_uid,nfs_gid) is enough to identify the nfs connection. Please remove a duplicated pair.
Not really. The second pair uit_t/gid_t is necessary for storing the looked up uid/gid values. The qemu NFS driver accepts just numerics and we must not try to look up uids/gids in the JSON prop generator.
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- docs/formatdomain.rst | 1 + docs/schemas/domaincommon.rng | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..40a1a3c1e2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2601,6 +2601,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 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
On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- docs/formatdomain.rst | 1 + docs/schemas/domaincommon.rng | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..40a1a3c1e2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2601,6 +2601,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 2049
Mention the feature introduced version here.
======== ======================================================= ============================================================ ================
And provide the nfs disk examples in the chapter hard-drives-floppy-disks-cdroms of formatdomain
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
On Thu, Dec 10, 2020 at 10:37 PM Han Han <hhan@redhat.com> wrote:
On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..40a1a3c1e2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2601,6 +2601,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 2049
Mention the feature introduced version here.
What version would we specify? QEMU's version 2.9.0 patch where they introduce NFS support? The Libvirt current version 6.10.0 patch? A future patch number (e.g. Libvirt version 6.11.0 or similar)? We were unclear on what exact number goes here, but we were planning on putting it into the space parallel to the :since: in the gluster line.
On 12/14/20 5:57 PM, Ryan Gahagan wrote:
On Thu, Dec 10, 2020 at 10:37 PM Han Han <hhan@redhat.com <mailto:hhan@redhat.com>> wrote:
On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan <rgahagan@cs.utexas.edu <mailto:rgahagan@cs.utexas.edu>> wrote:
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..40a1a3c1e2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2601,6 +2601,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 2049
Mention the feature introduced version here.
What version would we specify? QEMU's version 2.9.0 patch where they introduce NFS support? The Libvirt current version 6.10.0 patch? A future patch number (e.g. Libvirt version 6.11.0 or similar)? We were unclear on what exact number goes here, but we were planning on putting it into the space parallel to the :since: in the gluster line.
You would put the version that libvirt will be at when the feature is in released code, so if it was pushed today, that would be 7.0.0. (Sometimes we also indicate a minimum qemu or kernel version that's required, which is more useful if a feature is just newly supported in qemu/kernel but over time it just becomes extra numbers to confuse people. In this case, my opinion would be that the qemu version is old enough that it's not necessary to mention it.)
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 23415b323c..265c7584a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7844,6 +7844,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, @@ -9208,6 +9225,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); @@ -24761,6 +24781,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 | 42 +++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b224a550f3..f93f675262 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -674,6 +674,36 @@ 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, + "i:user", src->nfs_uid, + "i:group", src->nfs_gid, + "a:server", &server, NULL)); + + return ret; +} + + static virJSONValuePtr qemuBlockStorageSourceGetCURLProps(virStorageSourcePtr src, bool onlytarget) @@ -1181,6 +1211,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 +2535,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 b5a4a22ed3..64ebfb5812 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9589,6 +9589,45 @@ 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) + { + int err = 0; + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) + return 0; + + if ((virStorageNetProtocol) src->protocol != VIR_STORAGE_NET_PROTOCOL_NFS) + return 0; + + if (src->nfs_user) { + err = virGetUserID(src->nfs_user, &src->nfs_uid); + } else { + /* TODO: Provide hypervisor default value */ + } + + if (err < 0) + return -1; + + if (src->nfs_group) { + err = virGetGroupID(src->nfs_group, &src->nfs_gid); + } else { + /* TODO: Provide hypervisor default value */ + } + + if (err < 0) + return -1; + + return 0; +} + + /* qemuProcessPrepareStorageSourceTLS: * @source: source for a disk * @cfg: driver configuration @@ -10400,6 +10439,9 @@ qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDefPtr disk, priv) < 0) return -1; + if (qemuDomainPrepareStorageSourceNFS(src) < 0) + return -1; + return 0; } -- 2.29.0
On Thu, Dec 10, 2020 at 14:00:06 -0600, Ryan Gahagan wrote:
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/qemu/qemu_block.c | 42 +++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b224a550f3..f93f675262 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -674,6 +674,36 @@ 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, + "i:user", src->nfs_uid, + "i:group", src->nfs_gid, + "a:server", &server, NULL));
There's also virJSONValueObjectAdd, that might come in handy given my comment below.
+ + return ret; +}
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b5a4a22ed3..64ebfb5812 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9589,6 +9589,45 @@ 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) + { + int err = 0; + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) + return 0; + + if ((virStorageNetProtocol) src->protocol != VIR_STORAGE_NET_PROTOCOL_NFS) + return 0; + + if (src->nfs_user) { + err = virGetUserID(src->nfs_user, &src->nfs_uid);
You can return here directly if virGetUserID returns -1. No need for the extra vriable.
+ } else { + /* TODO: Provide hypervisor default value */
The best bet is to actually avoid formatting the user/group members formatting towards qemu. Unfortunately 0 is a very valid uid, so you'll probably need to use -1 to signal that it's the default.
+ if (err < 0) + return -1;
On Fri, Dec 11, 2020 at 3:30 AM Peter Krempa <pkrempa@redhat.com> wrote:
There's also virJSONValueObjectAdd, that might come in handy given my comment below.
[...]
The best bet is to actually avoid formatting the user/group members formatting towards qemu.
Unfortunately 0 is a very valid uid, so you'll probably need to use -1 to signal that it's the default.
We wanted to make sure we understood what you meant here. We've changed our code to store -1 into the nfs_uid and gid whenever the default is assumed, and only called virJSONValueObjectAdd to put the group and user integer properties into the JSON object if the uid and gid are not -1 (i.e. not defaults). This would leave them as "undefined" values in the JSON object. Is this what QEMU accepts when the default is needed, or have we misunderstood?
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/util/virstoragefile.c | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cff6dabd9e..098ec80542 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3805,6 +3805,47 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, } +static int +virStorageSourceParseBackingJSONNFS(virStorageSourcePtr src, + virJSONValuePtr json, + const char *jsonstr G_GNUC_UNUSED, + int opaque G_GNUC_UNUSED) +{ + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + int gotUID = virJSONValueObjectGetNumberInt(json, "user", (int *)(&src->nfs_uid)); + int gotGID = virJSONValueObjectGetNumberInt(json, "group", (int *)(&src->nfs_gid)); + + if (!vdisk_id || !server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' or '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->path = g_strdup(vdisk_id); + + 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 +3905,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 Thu, Dec 10, 2020 at 14:00:07 -0600, Ryan Gahagan wrote:
Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu> --- src/util/virstoragefile.c | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cff6dabd9e..098ec80542 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3805,6 +3805,47 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, }
+static int +virStorageSourceParseBackingJSONNFS(virStorageSourcePtr src, + virJSONValuePtr json, + const char *jsonstr G_GNUC_UNUSED, + int opaque G_GNUC_UNUSED) +{ + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
I'm fairly sure that this is a 'vxhs' protocol property. You'd probably notice it if you'd add tests in the same commit.
+ virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + int gotUID = virJSONValueObjectGetNumberInt(json, "user", (int *)(&src->nfs_uid));
You should not typecast the pointers here since it's not guaranteed that the uid_t/gid_t type is the same as an integer. Additionally storing this only in the nfs_uid field will actually not show up in the VM xml once parsed. You actually need to populate the string variants with the uid number with + prepended so that the XML conversion works.
+ int gotGID = virJSONValueObjectGetNumberInt(json, "group", (int *)(&src->nfs_gid)); + + if (!vdisk_id || !server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' or 'server' attribute in " + "JSON backing definition for NFS volume"));
Our coding style mandates that error messages in new code should be on a single line.
+ 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->path = g_strdup(vdisk_id); + + 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,
On Fri, Dec 11, 2020 at 3:24 AM Peter Krempa <pkrempa@redhat.com> wrote:
On Thu, Dec 10, 2020 at 14:00:07 -0600, Ryan Gahagan wrote:
+ virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + int gotUID = virJSONValueObjectGetNumberInt(json, "user", (int *)(&src->nfs_uid));
You should not typecast the pointers here since it's not guaranteed that the uid_t/gid_t type is the same as an integer. Additionally storing this only in the nfs_uid field will actually not show up in the VM xml once parsed. You actually need to populate the string variants with the uid number with + prepended so that the XML conversion works.
The reason we use this hacky integer pointer cast is because the virJSONValueObjectGetNumberInt method expects an int * as its thir parameter, and when we tried to use &src->nfs_uid or gid directly we got a compile error for type mismatch. This cast was the only way we could find to work around this easily other than changing the _virStorageSource parameters to be explicit int type, but then the virGetUserID and GroupID methods cause the opposite type mismatch error because they return uid_t and gid_t values. How should we actually get these numbers out if not for this cast?
On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan <rgahagan@cs.utexas.edu> wrote:
This is a request for comments on our patch for Issue 90. We didn't add in the test yet because we've had a ton of trouble getting the testing suite to work with our new test files, so they've omitted for now. We also did not add any logic to the attach-disk method yet because we wanted to focus on this set of changes first.
A tiny suggestion: You can use git-send-email with --cover-letter option to send a patch series with a cover letter instead of an independent cover letter email.
On Thu, Dec 10, 2020 at 14:00:01 -0600, Ryan Gahagan wrote:
This is a request for comments on our patch for Issue 90. We didn't add in the test yet because we've had a ton of trouble getting the testing suite to work with our new test files, so they've omitted for now. We also did not add any logic to the attach-disk method yet because we
attach-disk is covered by what you have. The 'prepare' step is executed already.
wanted to focus on this set of changes first.
What you have looks mostly okay. Obviously you must add the tests for the final submission. You also need to add a check to 'qemuDomainValidateStorageSource' that the NFS protocol is used only when the current qemu supports QEMU_CAPS_BLOCKDEV since only the blockdev portion is implemented. I'm not sure what your issues with tests are since you didn't specify them closer, but note that tests must not rely on host state. That means that the translation of uids must not be done in tests.
participants (5)
-
Han Han -
Laine Stump -
Peter Krempa -
Ryan Gahagan -
Ryan Gahagan