On Tue, Dec 29, 2020 at 15:21:24 -0600, Ryan Gahagan wrote: [...] Please add comments for nfs_uid and nfs_gid which state that the variables hold the converted/looked up uid/gid which is used at runtinme . This will help any further reader of the code see what they are used for and that they are not actually based on the configuration but are rather runtime (such as when we'll be adding driver-specific private data for virStorageSource).

On Tue, Dec 29, 2020 at 03:21:25PM -0600, Ryan Gahagan wrote: I'd be inclined to call this <identity> not <nfs> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Tue, Dec 29, 2020 at 15:21:25 -0600, Ryan Gahagan wrote: The docs is missing any description of the user/group fields. Please include the syntax as well. Additionally we tend to include any caveats of the initial implementation. In this case you should include that NFS-backed disks work only with TCP and port must be omitted. You've dropped the code which sets the default port so this isn't accurate. This will need changes after Daniel's suggestion. Note that 'unsignedInt' doesn't actually allow the '+' prefix. But both numbers and strings started with + are covered by genericName, so the choice isn't required. > + </choice> > + </attribute> > + </optional> > + <optional> > + <attribute name="group"> > + <choice> > + <ref name="unsignedInt"/> Same here. This allows also 'port' and non TCP transports. It's okay to simplify the schema though and use the generic type. The code will need to reject those when we'll validate whether the disk is possible to represent for qemu.

On Tue, Dec 29, 2020 at 15:21:26 -0600, Ryan Gahagan wrote: This patch can be merged into the previous one if you want. virBufferEscapeString has a special corner-case that it doesn't format anything if the third argument is NULL, so the explicit check is not necessary. With the code simplified: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On qemu-5.2.0-4.fc34.x86_64, libvirt v6.10.0-262-gbed50bcbbb with this patch series, by following script: #!/bin/bash - set -o nounset # Treat unset variables as an error VM=test NFS_EXPORT=/tmp/nfs IMG=disk1 DISK_XML=/tmp/nfs.xml mkdir -p $NFS_EXPORT echo "$NFS_EXPORT *(rw,no_root_squash,insecure)" > /etc/exports systemctl restart nfs-server qemu-img create nfs://localhost"$NFS_EXPORT/$IMG" 100M chmod 777 $NFS_EXPORT -R virsh start $VM --console echo "<disk type='network' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source protocol='nfs' name='$NFS_EXPORT/$IMG'> <host name='localhost'/> <nfs user='+107' group='+107'/> </source> <target dev='vdb' bus='virtio'/> </disk>" > $DISK_XML virsh attach-device $VM $DISK_XML virsh console $VM virsh detach-device $VM $DISK_XML virsh console $VM virsh destroy $VM virsh attach-device $VM $DISK_XML --config virsh start $VM --console virsh destroy $VM virsh detach-device $VM $DISK_XML --config It works for me. All attach/detach tests have passed. On Wed, Dec 30, 2020 at 5:23 AM Ryan Gahagan <rgahagan(a)cs.utexas.edu&gt; wrote: -- Tested-by: Han Han <hhan(a)redhat.com&gt;

Signed-off-by: Ryan Gahagan <rgahagan(a)cs.utexas.edu&gt; --- src/util/virstoragefile.c | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b5a5f267b9..ee8e31ce58 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3805,6 +3805,56 @@ 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 uidStore = -1; + int gidStore = -1; + int gotUID = virJSONValueObjectGetNumberInt(json, "user", &uidStore); + int gotGID = virJSONValueObjectGetNumberInt(json, "group", &gidStore); + g_auto(virBuffer) userBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) groupBuf = VIR_BUFFER_INITIALIZER; + + 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->path = g_strdup(virJSONValueObjectGetString(json, "path")); + + src->nfs_uid = (uid_t) uidStore; + src->nfs_gid = (gid_t) gidStore; + + virBufferAsprintf(&userBuf, "+%d", src->nfs_uid); + virBufferAsprintf(&groupBuf, "+%d", src->nfs_gid); + src->nfs_user = g_strdup(virBufferCurrentContent(&userBuf)); + src->nfs_group = g_strdup(virBufferCurrentContent(&groupBuf)); + + 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 +3914,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.2

Signed-off-by: Ryan Gahagan <rgahagan(a)cs.utexas.edu&gt; --- tests/qemuxml2argvdata/disk-network-nfs.args | 1 + .../disk-network-nfs.x86_64-latest.args | 56 +++++++++++++++++++ tests/qemuxml2argvdata/disk-network-nfs.xml | 48 ++++++++++++++++ tests/qemuxml2argvtest.c | 1 + ...isk-network-nfs-inactive.x86_64-latest.xml | 54 ++++++++++++++++++ .../disk-network-nfs.x86_64-latest.xml | 54 ++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/virstoragetest.c | 13 +++++ 8 files changed, 228 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.args create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-nfs-inactive.x86_64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml diff --git a/tests/qemuxml2argvdata/disk-network-nfs.args b/tests/qemuxml2argvdata/disk-network-nfs.args new file mode 100644 index 0000000000..fdc2941925 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-nfs.args @@ -0,0 +1 @@ +qemu_nfs diff --git a/tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args b/tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args new file mode 100644 index 0000000000..b0bc83bfc0 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args @@ -0,0 +1,56 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-cpu qemu64 \ +-m 214 \ +-object memory-backend-ram,id=pc.ram,size=224395264 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-blockdev '{"driver":"nfs","server":{"host":"example.com","type":"inet"},\ +"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"}' \ +-blockdev '{"node-name":"libvirt-3-format","read-only":false,\ +"cache":{"direct":true,"no-flush":false},"driver":"raw",\ +"file":"libvirt-3-storage"}' \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=libvirt-3-format,\ +id=virtio-disk0,bootindex=1,write-cache=on,\ +serial=eb90327c-8302-4725-9e1b-4e85ed4dc251 \ +-blockdev '{"driver":"nfs","server":{"host":"example.org","type":"inet"},\ +"path":"/backing/store/nfs","user":1234,"group":5678,\ +"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2",\ +"file":"libvirt-2-storage","backing":null}' \ +-blockdev '{"driver":"gluster","volume":"Volume2","path":"Image",\ +"server":[{"type":"unix","path":"/path/to/sock"}],"debug":4,\ +"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",\ +"file":"libvirt-1-storage","backing":"libvirt-2-format"}' \ +-device virtio-blk-pci,bus=pci.0,addr=0x2,drive=libvirt-1-format,\ +id=virtio-disk1 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/disk-network-nfs.xml b/tests/qemuxml2argvdata/disk-network-nfs.xml new file mode 100644 index 0000000000..67d2843e01 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-nfs.xml @@ -0,0 +1,48 @@ +<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='+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 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='qcow2'/> + <source protocol='nfs' name='/backing/store/nfs'> + <host name='example.org'/> + <nfs user='+1234' group='+5678'/> + </source> + <backingStore/> + </backingStore> + <target dev='vdb' bus='virtio'/> + </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> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 96a2b95331..af00feab61 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1242,6 +1242,7 @@ mymain(void) DO_TEST_CAPS_VER("disk-network-source-auth", "2.12.0"); DO_TEST_CAPS_LATEST("disk-network-source-auth"); DO_TEST("disk-network-vxhs", QEMU_CAPS_VXHS); + DO_TEST_CAPS_LATEST("disk-network-nfs"); driver.config->vxhsTLS = 1; driver.config->nbdTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea"); driver.config->vxhsTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea"); diff --git a/tests/qemuxml2xmloutdata/disk-network-nfs-inactive.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-network-nfs-inactive.x86_64-latest.xml new file mode 100644 index 0000000000..4728f5ee70 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-network-nfs-inactive.x86_64-latest.xml @@ -0,0 +1,54 @@ +<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> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <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='+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 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'> + <format type='qcow2'/> + <source protocol='nfs' name='/backing/store/nfs'> + <host name='example.org' port='2049'/> + <nfs user='+1234' group='+5678'/> + </source> + <backingStore/> + </backingStore> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml new file mode 100644 index 0000000000..9f03766810 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml @@ -0,0 +1,54 @@ +<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> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <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='+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 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='qcow2'/> + <source protocol='nfs' name='/backing/store/nfs'> + <host name='example.org' port='2049'/> + <nfs user='+1234' group='+5678'/> + </source> + <backingStore/> + </backingStore> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 1968be6782..856a52b65c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -302,6 +302,7 @@ mymain(void) DO_TEST("disk-network-source-auth", NONE); DO_TEST("disk-network-sheepdog", NONE); DO_TEST("disk-network-vxhs", NONE); + DO_TEST_CAPS_LATEST("disk-network-nfs"); DO_TEST("disk-network-tlsx509-nbd", NONE); DO_TEST("disk-network-tlsx509-vxhs", NONE); DO_TEST("disk-nvme", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_QCOW2_LUKS); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2e466ecb99..08e2723235 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1630,6 +1630,19 @@ mymain(void) "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" " <host name='example.com' port='9999'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"nfs\"," + "\"user\":2," + "\"group\":9," + "\"path\":\"/foo/bar/baz\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"2049\"" + "}" + "}" + "}", + "<source protocol='nfs' name='/foo/bar/baz'>\n" + " <host name='example.com' port='2049'/>\n" + " <nfs user='+2' group='+9'/>\n" + "</source>\n"); TEST_BACKING_PARSE_FULL("json:{ \"driver\": \"raw\"," "\"offset\": 10752," "\"size\": 4063232," -- 2.29.2

On Wed, Dec 30, 2020 at 5:24 AM Ryan Gahagan <rgahagan(a)cs.utexas.edu&gt; wrote: It is hard to know what the issue 90 actual means. Please add a ref link for it. For example: Per issue 90[1], .... .... Reference: [1]: https://gitlab.com/libvirt/libvirt/-/issues/90

On Tue, Dec 29, 2020 at 15:21:23 -0600, Ryan Gahagan wrote: [...] [...] I don't think we want to add a default port since the qemu code will not accept it at all.