[PATCH v2 7/7] tests: Added tests for NFS disk protocol

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 .../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 +
 6 files changed, 214 insertions(+)
 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.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args
new file mode 100644
index 00..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 00..df7b104a8e
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-nfs.xml
@@ -0,0 +1,48 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-x86_64
+
+  
+  
+
+
+  
+  
+  eb90327c-8302-4725-9e1b-4e85ed4dc251
+  
+
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+  
+

[PATCH v2 4/7] conf: Added NFS XML format/parse methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 src/conf/domain_conf.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 453e06491e..96ee009058 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 nfsIdentityNode = NULL;
+VIR_XPATH_NODE_AUTORESTORE(ctxt);
+
+ctxt->node = node;
+
+if ((nfsIdentityNode = virXPathNode("./identity", ctxt))) {
+src->nfs_user = virXMLPropString(nfsIdentityNode, "user");
+src->nfs_group = virXMLPropString(nfsIdentityNode, "group");
+}
+}
+
+
 static int
 virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode,
   xmlXPathContextPtr ctxt,
@@ -8250,6 +8267,9 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
 if (virDomainStorageNetworkParseHosts(node, ctxt, >hosts, 
>nhosts) < 0)
 return -1;
 
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS)
+virDomainStorageNetworkParseNFS(node, ctxt, src);
+
 virStorageSourceNetworkAssignDefaultPorts(src);
 
 virStorageSourceInitiatorParseXML(ctxt, >initiator);
@@ -23851,6 +23871,17 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
 virBufferAddLit(childBuf, "/>\n");
 }
 
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS &&
+(src->nfs_user || src->nfs_group)) {
+virBufferAddLit(childBuf, "nfs_user);
+virBufferEscapeString(childBuf, " group='%s'", src->nfs_group);
+
+virBufferAddLit(childBuf, "/>\n");
+}
+
+
 virBufferEscapeString(childBuf, "\n", src->snapshot);
 virBufferEscapeString(childBuf, "\n", src->configFile);
 
-- 
2.29.2

[PATCH v2 5/7] qemu: Added NFS JSON props methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 src/qemu/qemu_block.c  | 67 +++-
 src/qemu/qemu_domain.c | 70 ++
 2 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index b224a550f3..cef2f7d050 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -574,6 +574,29 @@ 
qemuBlockStorageSourceBuildJSONInetSocketAddress(virStorageNetHostDefPtr host)
 }
 
 
+/**
+ * qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
+ * @host: the virStorageNetHostDefPtr definition to build
+ *
+ * Formats @hosts into a json object conforming to the 'NFSServer' type
+ * in qemu.
+ *
+ * Returns a virJSONValuePtr for a single server.
+ */
+static virJSONValuePtr
+qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
+{
+virJSONValuePtr ret = NULL;
+
+ignore_value(virJSONValueObjectCreate(,
+  "s:host", host->name,
+  "s:type", "inet",
+  NULL));
+
+return ret;
+}
+
+
 /**
  * qemuBlockStorageSourceBuildHostsJSONInetSocketAddress:
  * @src: disk storage source
@@ -674,6 +697,38 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src,
 }
 
 
+static virJSONValuePtr
+qemuBlockStorageSourceGetNFSProps(virStorageSourcePtr src)
+{
+g_autoptr(virJSONValue) server = NULL;
+virJSONValuePtr ret = NULL;
+
+if (!(server = qemuBlockStorageSourceBuildJSONNFSServer(>hosts[0])))
+return NULL;
+
+/* NFS disk specification example:
+ * { driver:"nfs",
+ *   user: "0",
+ *   group: "0",
+ *   path: "/foo/bar/baz",
+ *   server: {type:"tcp", host:"1.2.3.4"}}
+ */
+ignore_value(virJSONValueObjectCreate(,
+  "a:server", ,
+  "S:path", src->path, 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 +1236,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 +2560,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 d91c32b2c5..fa427a1087 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4626,6 +4626,37 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
 return -1;
 }
 
+if (actualType == VIR_STORAGE_TYPE_NETWORK &&
+src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS) {
+/* NFS protocol may only be used if current QEMU supports blockdev */
+if (!blockdev) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'nfs' protocol is not supported with this QEMU 
binary"));
+return -1;
+}
+
+/* NFS protocol must have exactly one host */
+if (src->nhosts != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'nfs' protocol requires the usage of exactly one 
host"));
+return -1;
+}
+
+/* NFS can only use a TCP protocol */
+if (src->hosts[0].transport != VIR_STORAGE_NET_HOST_TRANS_TCP) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'nfs' host mus

[PATCH v2 3/7] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 docs/formatdomain.rst | 24 ++--
 docs/schemas/domaincommon.rng | 32 
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 1189795974..4402c52461 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2370,6 +2370,14 @@ paravirtualized driver is specified via the ``disk`` 
element.


  
+ 
+   
+   
+ 
+ 
+   
+   
+ 
  


@@ -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,12 +2609,15 @@ 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 daemonone or 
more ( :since:`Since 2.1.0` ), just one prior to that 24007
   vxhs a server running Veritas HyperScale daemon  only 
one 
+  nfs  a server running Network File Systemonly 
one ( :since:`Since 7.0.0` )must be omitted
    === 
 
 
   gluster supports "tcp", "rdma", "unix" as valid values for the transport
   attribute. nbd supports "tcp" and "unix". Others only support "tcp". If
   nothing is specified, "tcp" is assumed. If the transport is "unix", the
-  socket attribute specifies the path to an AF_UNIX socket.
+  socket attribute specifies the path to an AF_UNIX socket. nfs only
+  supports the use of a "tcp" transport, and does not support using a
+  port at all so it must be omitted.
 
``snapshot``
   The ``name`` attribute of ``snapshot`` element can optionally specify an
@@ -2683,6 +2694,15 @@ paravirtualized driver is specified via the ``disk`` 
element.
``timeout``
   Specifies the connection timeout for protocols which support it. Note 
that
   '0' is considered as if the value is not provided. :since:`Since 6.2.0`
+   ``identity``
+  When using an ``nfs`` protocol, this is used to provide information on 
the
+  configuration of the user and group. The element has two attributes,
+  ``user`` and ``group``. The user can provide these elements as user or
+  group strings, or as user and group ID numbers directly if the string
+  is formatted using a "+" at the beginning of the ID number. If either
+  of these attributes is omitted, then that field is assumed to be the
+  default value for the current system. If both ``user`` and ``group``
+  are intended to be default, then the entire element may be omitted.
 
For a "file" or "volume" disk type which represents a cdrom or floppy (the
``device`` attribute), it is possible to define policy what to do with the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 24b4994670..c3a4dd8ebc 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1774,6 +1774,21 @@
 
   
 
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
@@ -2039,6 +2054,22 @@
 
   
 
+  
+
+  
+
+  
+nfs
+  
+
+
+
+
+
+  
+
+  
+
   
 
   network
@@ -2053,6 +2084,7 @@
   
   
   
+  
 
   
 
-- 
2.29.2

[PATCH v2 2/7] util: Added nfs params to virStorageSource

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 src/util/virstoragefile.c | 8 
 src/util/virstoragefile.h | 8 
 2 files changed, 16 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1176630282..3097e11984 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2293,6 +2293,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();
 }
 
@@ -2533,6 +2538,9 @@ virStorageSourceClear(virStorageSourcePtr def)
 
 VIR_FREE(def->ssh_user);
 
+VIR_FREE(def->nfs_user);
+VIR_FREE(def->nfs_group);
+
 virStorageSourceInitiatorClear(>initiator);
 
 /* clear everything except the class header as the object APIs
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e1fbbafe26..47d901a478 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -385,6 +385,14 @@ 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;
+
+/* nfs_user and nfs_group store the strings passed in by the user for NFS 
params.
+ * nfs_uid and nfs_gid represent the converted/looked up ID numbers which 
are used
+ * during run time, and are not based on the configuration */
+char *nfs_user;
+char *nfs_group;
+uid_t nfs_uid;
+gid_t nfs_gid;
 };
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);
-- 
2.29.2

[PATCH v2 1/7] conf: Add NFS disk protocol

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 02f956ce48..be06042cce 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..d91c32b2c5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9630,6 +9630,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,
   

PATCH v2: Implement NFS Protocol

[Ryan Gahagan]

Per Issue 90[1], QEMU supports using an NFS disk but Libvirt had no
support for attaching a disk of this type. This patch series adds in
this capability as well as the tests necessary to make sure it works
correctly.

The changes in this second version over the previous patch series for
this issue primarily address feedback. Documentation was updated and the
XML  element has been renamed as an  element. The code
for checking user XML for an NFS protocol has been relocated to the
validation method and other pieces of code were refactored per
suggestion. Tests have been altered to reflect the new XML element and
the fact that NFS does not allow hosts to have a port.

References:
[1]: https://gitlab.com/libvirt/libvirt/-/issues/90

[PATCH v2 6/7] util: Added a backing store NFS parser

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 src/util/virstoragefile.c | 49 +++
 tests/virstoragetest.c| 13 +++
 2 files changed, 62 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3097e11984..d67f0f2c3f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3652,6 +3652,54 @@ 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", );
+int gotGID = virJSONValueObjectGetNumberInt(json, "group", );
+
+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"));
+if (!src->path) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("missing 'path' attribute in JSON backing definition 
for NFS volume"));
+return -1;
+}
+
+src->nfs_user = g_strdup_printf("+%d", uidStore);
+src->nfs_group = g_strdup_printf("+%d", gidStore);
+
+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,
@@ -3711,6 +3759,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},
 };
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index a376154def..86c7cd910c 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1624,6 +1624,19 @@ mymain(void)
"\n"
"  \n"
"\n");
+TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"nfs\","
+   "\"user\":2,"
+   "\"group\":9,"
+   "\"path\":\"/foo/bar/baz\","
+   "\"server\": {  \"host\":\"example.com\","
+  "\"type\":\"inet\""
+   "}"
+  "}"
+"}",
+   "\n"
+   "  \n"
+   "  \n"
+   "\n");
 TEST_BACKING_PARSE_FULL("json:{ \"driver\": \"raw\","
 "\"offset\": 10752,"
 "\"size\": 4063232,"
-- 
2.29.2

Re: [PATCH 6/7] util: Added a backing store NFS parser

[Ryan Gahagan]

On Mon, Jan 4, 2021 at 8:41 AM Peter Krempa  wrote:

> On Tue, Dec 29, 2020 at 15:21:28 -0600, Ryan Gahagan wrote:
> > +src->nfs_uid = (uid_t) uidStore;
> > +src->nfs_gid = (gid_t) gidStore;
>
> This function must not fill in runtime data, just configuration. I
> presume you did this to silence tests but you'll need to add a hack into
> the test code rather than abusing this to fill runtime data.
>
> Ideally in the future the runtime data will be split off into an opaque
> sub-object so it will not be accessible in this code. Don't touch
> nfs_uid/nfs_gid in this function at all.
>

We removed this code (specifically these assignments to nfs_uid and
nfs_gid) and did all the other refactors requested for this method.
Interestingly, the virstoragetest which tests this code still passes.
However, we're unaware of any "hack" in our test code which actually fixes
the missing uid and gid values. We're worried that this might indicate a
problem with our test. Where should this storage when parsing backing store
data actually be done?

For reference, here is the test
<https://gitlab.com/bschoney/libvirt/-/blob/issue-90-rt2/tests/virstoragetest.c#L1633>
(on our branch after the changes) which is passing. Here
<https://gitlab.com/bschoney/libvirt/-/blob/issue-90-rt2/src/util/virstoragefile.c#L3809>
is
the parse backing store method after the changes. Maybe we're just
overreacting but we figured asking to make sure would be better than
submitting a new patch only to find a problem.

Re: [PATCH 3/7] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

On Mon, Jan 4, 2021 at 8:24 AM Peter Krempa  wrote:

> On Tue, Dec 29, 2020 at 15:21:25 -0600, Ryan Gahagan wrote:
> > +  
> > +
> > +  
> > +
> > +  
> > +nfs
> > +  
> > +
> > +
> > +
> > +
>
> 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.
>

Just to be 100% clear, does this mean we can leave the
diskSourceNetworkProtocolNFS element definition as is and simply enforce
the port omission/tcp transport in the code? Or would you prefer that we
re-write the schema to use a custom variety of the diskSourceNetworkHost
which only supports tcp and has no port option?

[PATCH 7/7] tests: Added tests for NFS disk protocol

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 00..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 00..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 00..67d2843e01
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-nfs.xml
@@ -0,0 +1,48 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-94

[PATCH 6/7] util: Added a backing store NFS parser

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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", );
+int gotGID = virJSONValueObjectGetNumberInt(json, "group", );
+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(, "+%d", src->nfs_uid);
+virBufferAsprintf(, "+%d", src->nfs_gid);
+src->nfs_user = g_strdup(virBufferCurrentContent());
+src->nfs_group = g_strdup(virBufferCurrentContent());
+
+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

[PATCH 1/7] conf: Add NFS disk protocol

[Ryan Gahagan]

Per Issue 90, Libvirt does not support attaching an NFS disk even though
QEMU has added support for it. This series of patches seeks to implement
this support in Libvirt and begins by adding in flags for an NFS disk.

Signed-off-by: Ryan Gahagan 
---
 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 bfb6e23942..d91c32b2c5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9630,6 +9630,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:
 

[PATCH 5/7] qemu: Added NFS JSON props methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 src/qemu/qemu_block.c  | 79 +-
 src/qemu/qemu_domain.c | 47 +
 2 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index b224a550f3..5413a4f0e8 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -574,6 +574,35 @@ 
qemuBlockStorageSourceBuildJSONInetSocketAddress(virStorageNetHostDefPtr host)
 }
 
 
+/**
+ * qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
+ * @host: the virStorageNetHostDefPtr definition to build
+ *
+ * Formats @hosts into a json object conforming to the 'NFSServer' type
+ * in qemu.
+ *
+ * Returns a virJSONValuePtr for a single server.
+ */
+static virJSONValuePtr
+qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
+{
+virJSONValuePtr ret = NULL;
+
+if (host->transport != VIR_STORAGE_NET_HOST_TRANS_TCP) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("only TCP protocol can be converted to NFSServer"));
+return NULL;
+}
+
+ignore_value(virJSONValueObjectCreate(,
+  "s:host", host->name,
+  "s:type", "inet",
+  NULL));
+
+return ret;
+}
+
+
 /**
  * qemuBlockStorageSourceBuildHostsJSONInetSocketAddress:
  * @src: disk storage source
@@ -674,6 +703,44 @@ 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 = qemuBlockStorageSourceBuildJSONNFSServer(>hosts[0])))
+return NULL;
+
+/* NFS disk specification example:
+ * { driver:"nfs",
+ *   user: "0",
+ *   group: "0",
+ *   path: "/foo/bar/baz",
+ *   server: {type:"tcp", host:"1.2.3.4"}}
+ */
+ignore_value(virJSONValueObjectCreate(,
+  "a:server", ,
+  "S:path", src->path, 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 +1248,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 +2572,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 d91c32b2c5..8812df5138 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_PROTOCOL_NFS &&
+!blockdev) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'nfs' protocol is not supported with this QEMU 
binary"));
+}
+
 return 0;
 }
 
@@ -9590,6 +9598,42 @@ 
qemuProcessPrepareStorageSourceTLSNBD(virStorageSourcePtr src,
 }
 
 
+/* qemuPrepare

[PATCH 4/7] conf: Added NFS XML format/parse methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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, >hosts, 
>nhosts) < 0)
 return -1;
 
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS)
+virDomainStorageNetworkParseNFS(node, ctxt, src);
+
 virStorageSourceNetworkAssignDefaultPorts(src);
 
 virStorageSourceInitiatorParseXML(ctxt, >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_user)
+virBufferEscapeString(childBuf, " user='%s'", src->nfs_user);
+if (src->nfs_group)
+virBufferEscapeString(childBuf, " group='%s'", src->nfs_group);
+
+virBufferAddLit(childBuf, "/>\n");
+}
+
+
 virBufferEscapeString(childBuf, "\n", src->snapshot);
 virBufferEscapeString(childBuf, "\n", src->configFile);
 
-- 
2.29.2

[PATCH 3/7] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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.


  
+ 
+   
+   
+ 
+ 
+   
+   
+ 
  


@@ -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 daemonone or 
more ( :since:`Since 2.1.0` ), just one prior to that 24007
   vxhs a server running Veritas HyperScale daemon  only 
one 
+  nfs  a server running Network File Systemonly 
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 17e25f14f2..4ddbf13ec4 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1774,6 +1774,27 @@
 
   
 
+  
+
+  
+
+  
+
+
+  
+
+  
+  
+
+  
+
+
+  
+
+  
+
+  
+
   
 
   
@@ -2039,6 +2060,22 @@
 
   
 
+  
+
+  
+
+  
+nfs
+  
+
+
+
+
+
+  
+
+  
+
   
 
   network
@@ -2053,6 +2090,7 @@
   
   
   
+  
 
   
 
-- 
2.29.2

[PATCH 2/7] util: Added nfs params to virStorageSource

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 103dade0e7..b5a5f267b9 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();
 }
 
@@ -2686,6 +2691,9 @@ virStorageSourceClear(virStorageSourcePtr def)
 
 VIR_FREE(def->ssh_user);
 
+VIR_FREE(def->nfs_user);
+VIR_FREE(def->nfs_group);
+
 virStorageSourceInitiatorClear(>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.2

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa  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 "". 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?

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa  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
.

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  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.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

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. “”) 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?

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa  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.

[RFC PATCH 4/6] conf: Added NFS XML format/parse methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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, >hosts, 
>nhosts) < 0)
 return -1;
 
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS)
+virDomainStorageNetworkParseNFS(node, ctxt, src);
+
 virStorageSourceNetworkAssignDefaultPorts(src);
 
 virStorageSourceInitiatorParseXML(ctxt, >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_user)
+virBufferEscapeString(childBuf, " user='%s'", src->nfs_user);
+if (src->nfs_group)
+virBufferEscapeString(childBuf, " group='%s'", src->nfs_group);
+
+virBufferAddLit(childBuf, "/>\n");
+}
+
+
 virBufferEscapeString(childBuf, "\n", src->snapshot);
 virBufferEscapeString(childBuf, "\n", src->configFile);
 
-- 
2.29.0

[RFC PATCH 1/6] conf: Add NFS disk protocol

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 

[RFC PATCH 5/6] qemu: Added NFS JSON props methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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(>hosts[0])))
+return NULL;
+
+/* NFS disk specification example:
+ * { driver:"nfs",
+ *   user: "0",
+ *   group: "0",
+ *   server: {type:"tcp", host:"1.2.3.4", port:}}
+ */
+ignore_value(virJSONValueObjectCreate(,
+  "a: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, >nfs_uid) < 0)
+return -1;
+} else {
+/* -1 indicates default UID */
+src->nfs_uid = -1;
+}
+
+if (src->nfs_group) {
+if (virGetGroupID(src->nfs_group, >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

[RFC PATCH 6/6] util: Added a backing store NFS parser

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 
*)(>nfs_uid));
+int gotGID = virJSONValueObjectGetNumberInt(json, "group", (int 
*)(>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

RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

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:

  QEMUGuest1
  c7a5fdbd-edaf-9455-926a-d65c16db1809
  219136
  219136
  1
  
hvm

  
  
  destroy
  restart
  destroy
  
/usr/bin/qemu-system-x86_64

  
  


  
  
  eb90327c-8302-4725-9e1b-4e85ed4dc251
  






  


We have under tests/qemublocktest.c:
TEST_JSON_FORMAT_NET(“\n”
 “  \n”
 “  \n”
 "\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\””
   ”}”
  “}”
“}”,
   “\n”
   “  \n”
   “  \n”
   “\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.

[RFC PATCH 3/6] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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.


  
+ 
+   
+   
+ 
+ 
+   
+   
+ 
  


@@ -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 daemonone or 
more ( :since:`Since 2.1.0` ), just one prior to that 24007
   vxhs a server running Veritas HyperScale daemon  only 
one 
+  nfs  a server running Network File Systemonly 
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 @@
 
   
 
+  
+
+  
+
+  
+
+
+  
+
+  
+  
+
+  
+
+
+  
+
+  
+
+  
+
   
 
   
@@ -2039,6 +2060,22 @@
 
   
 
+  
+
+  
+
+  
+nfs
+  
+
+
+
+
+
+  
+
+  
+
   
 
   network
@@ -2053,6 +2090,7 @@
   
   
   
+  
 
   
 
-- 
2.29.0

[RFC PATCH 2/6] util: Added nfs params to virStorageSource

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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();
 }
 
@@ -2686,6 +2691,9 @@ virStorageSourceClear(virStorageSourcePtr def)
 
 VIR_FREE(def->ssh_user);
 
+VIR_FREE(def->nfs_user);
+VIR_FREE(def->nfs_group);
+
 virStorageSourceInitiatorClear(>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

Re: [RFC PATCH 6/6] util: Added a backing store NFS parser

[Ryan Gahagan]

On Fri, Dec 11, 2020 at 3:24 AM Peter Krempa  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
> *)(>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 >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?

Re: [RFC PATCH 5/6] qemu: added NFS JSON props methods

[Ryan Gahagan]

On Fri, Dec 11, 2020 at 3:30 AM Peter Krempa  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?

Re: [RFC PATCH 3/6] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

On Thu, Dec 10, 2020 at 10:37 PM Han Han  wrote:

> On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan 
> 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 
>> +  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.

Re: [RFC PATCH 1/6] compatibilities: added in flags for NFS support

[Ryan Gahagan]

On Fri, Dec 11, 2020 at 3:35 AM Peter Krempa  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?

[RFC PATCH 4/6] conf: Added NFS XML format/parse methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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, >hosts, 
>nhosts) < 0)
 return -1;
 
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS)
+virDomainStorageNetworkParseNFS(node, ctxt, src);
+
 virStorageSourceNetworkAssignDefaultPorts(src);
 
 virStorageSourceInitiatorParseXML(ctxt, >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_user)
+virBufferEscapeString(childBuf, " user='%s'", src->nfs_user);
+if (src->nfs_group)
+virBufferEscapeString(childBuf, " group='%s'", src->nfs_group);
+
+virBufferAddLit(childBuf, "/>\n");
+}
+
+
 virBufferEscapeString(childBuf, "\n", src->snapshot);
 virBufferEscapeString(childBuf, "\n", src->configFile);
 
-- 
2.29.0

[RFC PATCH 6/6] util: Added a backing store NFS parser

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 
*)(>nfs_uid));
+int gotGID = virJSONValueObjectGetNumberInt(json, "group", (int 
*)(>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

[RFC PATCH 2/6] util: Added nfs params to virStorageSource

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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();
 }
 
@@ -2686,6 +2691,9 @@ virStorageSourceClear(virStorageSourcePtr def)
 
 VIR_FREE(def->ssh_user);
 
+VIR_FREE(def->nfs_user);
+VIR_FREE(def->nfs_group);
+
 virStorageSourceInitiatorClear(>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

[RFC PATCH 5/6] qemu: added NFS JSON props methods

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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(>hosts[0])))
+return NULL;
+
+/* NFS disk specification example:
+ * { driver:"nfs",
+ *   user: "0",
+ *   group: "0",
+ *   server: {type:"tcp", host:"1.2.3.4", port:}}
+ */
+ignore_value(virJSONValueObjectCreate(,
+  "i:user", src->nfs_uid,
+  "i:group", src->nfs_gid,
+  "a: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, >nfs_uid);
+} else {
+/* TODO: Provide hypervisor default value */
+}
+
+if (err < 0)
+return -1;
+
+if (src->nfs_group) {
+err = virGetGroupID(src->nfs_group, >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

[RFC PATCH 1/6] compatibilities: added in flags for NFS support

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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,
   

[RFC PATCH 3/6] docs: added rng schema and formatdomain for NFS

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 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 daemonone or 
more ( :since:`Since 2.1.0` ), just one prior to that 24007
   vxhs a server running Veritas HyperScale daemon  only 
one 
+  nfs  a server running Network File Systemonly 
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 @@
 
   
 
+  
+
+  
+
+  
+
+
+  
+
+  
+  
+
+  
+
+
+  
+
+  
+
+  
+
   
 
   
@@ -2039,6 +2060,22 @@
 
   
 
+  
+
+  
+
+  
+nfs
+  
+
+
+
+
+
+  
+
+  
+
   
 
   network
@@ -2053,6 +2090,7 @@
   
   
   
+  
 
   
 
-- 
2.29.0

RFC PATCH: Issue 90

[Ryan Gahagan]

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.

[PATCH 25/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virsecret.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virsecret.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/util/virsecret.c b/src/util/virsecret.c
index 78a3b41bc5..9ed803d45b 100644
--- a/src/util/virsecret.c
+++ b/src/util/virsecret.c
@@ -67,27 +67,26 @@ virSecretLookupParseSecret(xmlNodePtr secretnode,
 {
 g_autofree char *uuid = NULL;
 g_autofree char *usage = NULL;
-int ret = -1;
 
 uuid = virXMLPropString(secretnode, "uuid");
 usage = virXMLPropString(secretnode, "usage");
 if (uuid == NULL && usage == NULL) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing secret uuid or usage attribute"));
-goto cleanup;
+return -1;
 }
 
 if (uuid && usage) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("either secret uuid or usage expected"));
-goto cleanup;
+return -1;
 }
 
 if (uuid) {
 if (virUUIDParse(uuid, def->u.uuid) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
_("invalid secret uuid '%s'"), uuid);
-goto cleanup;
+return -1;
 }
 def->type = VIR_SECRET_LOOKUP_TYPE_UUID;
 } else {
@@ -95,10 +94,7 @@ virSecretLookupParseSecret(xmlNodePtr secretnode,
 usage = NULL;
 def->type = VIR_SECRET_LOOKUP_TYPE_USAGE;
 }
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
-- 
2.29.0

[PATCH 04/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/viruri.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/viruri.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/util/viruri.c b/src/util/viruri.c
index 11753a0aef..704e5b2132 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -209,7 +209,7 @@ char *
 virURIFormat(virURIPtr uri)
 {
 xmlURI xmluri;
-char *tmpserver = NULL;
+g_autofree char *tmpserver = NULL;
 char *ret;
 
 memset(, 0, sizeof(xmluri));
@@ -245,8 +245,6 @@ virURIFormat(virURIPtr uri)
 }
 
  cleanup:
-VIR_FREE(tmpserver);
-
 return ret;
 }
 
-- 
2.29.0

[PATCH 19/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/vircgroupv1.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/vircgroupv1.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
-- 
2.29.0

[PATCH 14/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virlog.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virlog.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 6b7a4512e9..e12fd58831 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -505,8 +505,8 @@ virLogVMessage(virLogSourcePtr source,
va_list vargs)
 {
 static bool logInitMessageStderr = true;
-char *str = NULL;
-char *msg = NULL;
+g_autofree char *str = NULL;
+g_autofree char *msg = NULL;
 char timestamp[VIR_TIME_STRING_BUFLEN];
 size_t i;
 int saved_errno = errno;
@@ -601,10 +601,7 @@ virLogVMessage(virLogSourcePtr source,
  str, msg, (void *) STDERR_FILENO);
 }
 virLogUnlock();
-
  cleanup:
-VIR_FREE(str);
-VIR_FREE(msg);
 errno = saved_errno;
 }
 
-- 
2.29.0

[PATCH 29/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virlog.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virlog.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index e12fd58831..2bf606b8c5 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -528,7 +528,8 @@ virLogVMessage(virLogSourcePtr source,
 if (source->serial < virLogFiltersSerial)
 virLogSourceUpdate(source);
 if (priority < source->priority)
-goto cleanup;
+errno = saved_errno;
+return;
 
 /*
  * serialize the error message, add level and timestamp
@@ -601,8 +602,6 @@ virLogVMessage(virLogSourcePtr source,
  str, msg, (void *) STDERR_FILENO);
 }
 virLogUnlock();
- cleanup:
-errno = saved_errno;
 }
 
 
-- 
2.29.0

[PATCH 24/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virstorageencryption.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virstorageencryption.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c
index ed29b4cfa5..a330b79637 100644
--- a/src/util/virstorageencryption.c
+++ b/src/util/virstorageencryption.c
@@ -176,13 +176,12 @@ static int
 virStorageEncryptionInfoParseCipher(xmlNodePtr info_node,
 virStorageEncryptionInfoDefPtr info)
 {
-int ret = -1;
 g_autofree char *size_str = NULL;
 
 if (!(info->cipher_name = virXMLPropString(info_node, "name"))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("cipher info missing 'name' attribute"));
-goto cleanup;
+return -1;
 }
 
 if ((size_str = virXMLPropString(info_node, "size")) &&
@@ -190,22 +189,19 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr info_node,
 virReportError(VIR_ERR_XML_ERROR,
_("cannot parse cipher size: '%s'"),
size_str);
-goto cleanup;
+return -1;
 }
 
 if (!size_str) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("cipher info missing 'size' attribute"));
-goto cleanup;
+return -1;
 }
 
 info->cipher_mode = virXMLPropString(info_node, "mode");
 info->cipher_hash = virXMLPropString(info_node, "hash");
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
-- 
2.29.0

[PATCH 08/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virsecret.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virsecret.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/virsecret.c b/src/util/virsecret.c
index 54d6bbcb7c..78a3b41bc5 100644
--- a/src/util/virsecret.c
+++ b/src/util/virsecret.c
@@ -65,8 +65,8 @@ int
 virSecretLookupParseSecret(xmlNodePtr secretnode,
virSecretLookupTypeDefPtr def)
 {
-char *uuid;
-char *usage;
+g_autofree char *uuid = NULL;
+g_autofree char *usage = NULL;
 int ret = -1;
 
 uuid = virXMLPropString(secretnode, "uuid");
@@ -98,8 +98,6 @@ virSecretLookupParseSecret(xmlNodePtr secretnode,
 ret = 0;
 
  cleanup:
-VIR_FREE(uuid);
-VIR_FREE(usage);
 return ret;
 }
 
-- 
2.29.0

[PATCH 26/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virscsihost.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virscsihost.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c
index 2ce33e4cfb..3aecb3146f 100644
--- a/src/util/virscsihost.c
+++ b/src/util/virscsihost.c
@@ -113,7 +113,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
 host_link = g_strdup_printf("%s/%s", prefix, entry->d_name);
 
 if (virFileResolveLink(host_link, _path) < 0)
-goto cleanup;
+return ret;
 
 if (!strstr(host_path, parentaddr)) {
 VIR_FREE(host_link);
@@ -131,13 +131,13 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
 }
 
 if (virFileReadAll(unique_path, 1024, ) < 0)
-goto cleanup;
+return ret;
 
 if ((p = strchr(buf, '\n')))
 *p = '\0';
 
 if (virStrToLong_ui(buf, NULL, 10, _unique_id) < 0)
-goto cleanup;
+return ret;
 
 VIR_FREE(buf);
 
@@ -150,7 +150,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
 break;
 }
 
- cleanup:
 return ret;
 }
 
@@ -231,10 +230,8 @@ virSCSIHostGetNameByParentaddr(unsigned int domain,
_("Failed to find scsi_host using PCI '%s' "
  "and unique_id='%u'"),
parentaddr, unique_id);
-goto cleanup;
 }
 
- cleanup:
 return name;
 }
 
-- 
2.29.0

[PATCH 30/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virdnsmasq.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virdnsmasq.c | 46 +--
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index 5f477c976d..b41cdb8047 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -168,7 +168,6 @@ addnhostsWrite(const char *path,
 FILE *f;
 bool istmp = true;
 size_t i, j;
-int rc = 0;
 
 /* even if there are 0 hosts, create a 0 length file, to allow
  * for runtime addition.
@@ -179,58 +178,51 @@ addnhostsWrite(const char *path,
 if (!(f = fopen(tmp, "w"))) {
 istmp = false;
 if (!(f = fopen(path, "w"))) {
-rc = -errno;
-goto cleanup;
+return -errno;
 }
 }
 
 for (i = 0; i < nhosts; i++) {
 if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) {
-rc = -errno;
 VIR_FORCE_FCLOSE(f);
 
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return -errno;
 }
 
 for (j = 0; j < hosts[i].nhostnames; j++) {
 if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == 
EOF) {
-rc = -errno;
 VIR_FORCE_FCLOSE(f);
 
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return -errno;
 }
 }
 
 if (fputc('\n', f) == EOF) {
-rc = -errno;
 VIR_FORCE_FCLOSE(f);
 
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return -errno;
 }
 }
 
 if (VIR_FCLOSE(f) == EOF) {
-rc = -errno;
-goto cleanup;
+return -errno;
 }
 
 if (istmp && rename(tmp, path) < 0) {
-rc = -errno;
 unlink(tmp);
-goto cleanup;
+return -errno;
 }
 
- cleanup:
-return rc;
+return 0;
 }
 
 static int
@@ -366,7 +358,6 @@ hostsfileWrite(const char *path,
 FILE *f;
 bool istmp = true;
 size_t i;
-int rc = 0;
 
 /* even if there are 0 hosts, create a 0 length file, to allow
  * for runtime addition.
@@ -377,36 +368,31 @@ hostsfileWrite(const char *path,
 if (!(f = fopen(tmp, "w"))) {
 istmp = false;
 if (!(f = fopen(path, "w"))) {
-rc = -errno;
-goto cleanup;
+return -errno;
 }
 }
 
 for (i = 0; i < nhosts; i++) {
 if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) {
-rc = -errno;
 VIR_FORCE_FCLOSE(f);
 
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return -errno;
 }
 }
 
 if (VIR_FCLOSE(f) == EOF) {
-rc = -errno;
-goto cleanup;
+return -errno;
 }
 
 if (istmp && rename(tmp, path) < 0) {
-rc = -errno;
 unlink(tmp);
-goto cleanup;
+return -errno;
 }
 
- cleanup:
-return rc;
+return 0;
 }
 
 static int
@@ -681,16 +667,12 @@ dnsmasqCapsSetFromBuffer(dnsmasqCapsPtr caps, const char 
*buf)
 static int
 dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path)
 {
-int ret = -1;
 g_autofree char *buf = NULL;
 
 if (virFileReadAll(path, 1024 * 1024, ) < 0)
-goto cleanup;
-
-ret = dnsmasqCapsSetFromBuffer(caps, buf);
+return -1;
 
- cleanup:
-return ret;
+return dnsmasqCapsSetFromBuffer(caps, buf);
 }
 
 static int
-- 
2.29.0

[PATCH 15/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virlockspace.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virlockspace.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index b90e13f506..71d5dfb83e 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
@@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
-- 
2.29.0

[PATCH 27/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virrotatingfile.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virrotatingfile.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
index 6d8076e7c4..45dc66e94d 100644
--- a/src/util/virrotatingfile.c
+++ b/src/util/virrotatingfile.c
@@ -364,7 +364,6 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 size_t i;
 g_autofree char *nextpath = NULL;
 g_autofree char *thispath = NULL;
-int ret = -1;
 
 VIR_DEBUG("Rollover %s", file->basepath);
 if (file->maxbackup == 0) {
@@ -373,7 +372,7 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 virReportSystemError(errno,
  _("Unable to remove %s"),
  file->basepath);
-goto cleanup;
+return -1;
 }
 } else {
 nextpath = g_strdup_printf("%s.%zu", file->basepath, file->maxbackup - 
1);
@@ -391,7 +390,7 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 virReportSystemError(errno,
  _("Unable to rename %s to %s"),
  thispath, nextpath);
-goto cleanup;
+return -1;
 }
 
 VIR_FREE(nextpath);
@@ -401,9 +400,7 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 
 VIR_DEBUG("Rollover done %s", file->basepath);
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 
-- 
2.29.0

[PATCH 01/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virxml.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virxml.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index a3b819d85c..7df50e4b4d 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -696,8 +696,8 @@ catchXMLError(void *ctx, const char *msg G_GNUC_UNUSED, ...)
 unsigned int n, col;/* GCC warns if signed, because compared with 
sizeof() */
 int domcode = VIR_FROM_XML;
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-char *contextstr = NULL;
-char *pointerstr = NULL;
+g_autofree char *contextstr = NULL;
+g_autofree char *pointerstr = NULL;
 
 
 /* conditions for error printing */
@@ -763,9 +763,6 @@ catchXMLError(void *ctx, const char *msg G_GNUC_UNUSED, ...)
   contextstr,
   pointerstr);
 }
-
-VIR_FREE(contextstr);
-VIR_FREE(pointerstr);
 }
 
 /**
-- 
2.29.0

[PATCH 06/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virstoragefilebackend.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virstoragefilebackend.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/util/virstoragefilebackend.c b/src/util/virstoragefilebackend.c
index 2779b5c307..55c62b0212 100644
--- a/src/util/virstoragefilebackend.c
+++ b/src/util/virstoragefilebackend.c
@@ -51,7 +51,7 @@ virStorageFileLoadBackendModule(const char *name,
 const char *regfunc,
 bool forceload)
 {
-char *modfile = NULL;
+g_autofree char *modfile = NULL;
 int ret;
 
 if (!(modfile = virFileFindResourceFull(name,
@@ -64,8 +64,6 @@ virStorageFileLoadBackendModule(const char *name,
 
 ret = virModuleLoad(modfile, regfunc, forceload);
 
-VIR_FREE(modfile);
-
 return ret;
 }
 #endif /* WITH_STORAGE_DIR || WITH_STORAGE_FS || WITH_STORAGE_GLUSTER */
-- 
2.29.0

[PATCH 09/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virscsihost.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virscsihost.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c
index 969cdd9f79..2ce33e4cfb 100644
--- a/src/util/virscsihost.c
+++ b/src/util/virscsihost.c
@@ -95,12 +95,12 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH;
 struct dirent *entry = NULL;
 g_autoptr(DIR) dir = NULL;
-char *host_link = NULL;
-char *host_path = NULL;
+g_autofree char *host_link = NULL;
+g_autofree char *host_path = NULL;
 char *p = NULL;
 char *ret = NULL;
-char *buf = NULL;
-char *unique_path = NULL;
+g_autofree char *buf = NULL;
+g_autofree char *unique_path = NULL;
 unsigned int read_unique_id;
 
 if (virDirOpen(, prefix) < 0)
@@ -151,10 +151,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
 }
 
  cleanup:
-VIR_FREE(unique_path);
-VIR_FREE(host_link);
-VIR_FREE(host_path);
-VIR_FREE(buf);
 return ret;
 }
 
@@ -226,7 +222,7 @@ virSCSIHostGetNameByParentaddr(unsigned int domain,
unsigned int unique_id)
 {
 char *name = NULL;
-char *parentaddr = NULL;
+g_autofree char *parentaddr = NULL;
 
 parentaddr = g_strdup_printf("%04x:%02x:%02x.%01x", domain, bus, slot,
  function);
@@ -239,7 +235,6 @@ virSCSIHostGetNameByParentaddr(unsigned int domain,
 }
 
  cleanup:
-VIR_FREE(parentaddr);
 return name;
 }
 
-- 
2.29.0

[PATCH 17/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virfile.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virfile.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index f57272ca2f..38207f1948 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3328,9 +3328,9 @@ virFileIsSharedFixFUSE(const char *path,
 FILE *f = NULL;
 struct mntent mb;
 char mntbuf[1024];
-char *mntDir = NULL;
-char *mntType = NULL;
-char *canonPath = NULL;
+g_autofree char *mntDir = NULL;
+g_autofree char *mntType = NULL;
+g_autofree char *canonPath = NULL;
 size_t maxMatching = 0;
 int ret = -1;
 
@@ -3381,9 +3381,6 @@ virFileIsSharedFixFUSE(const char *path,
 
 ret = 0;
  cleanup:
-VIR_FREE(canonPath);
-VIR_FREE(mntType);
-VIR_FREE(mntDir);
 endmntent(f);
 return ret;
 }
-- 
2.29.0

[PATCH 21/30] util: remove cleanup label

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virutil.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virutil.c | 45 -
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index d4b864d5cb..52ab6c6e6d 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -773,7 +773,7 @@ virGetUserIDByName(const char *name, uid_t *uid, bool 
missing_ok)
 
 while ((rc = getpwnam_r(name, , strbuf, strbuflen, )) == ERANGE) {
 if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
-goto cleanup;
+return ret;
 }
 
 if (!pw) {
@@ -785,15 +785,13 @@ virGetUserIDByName(const char *name, uid_t *uid, bool 
missing_ok)
  name, g_strerror(rc));
 }
 
-ret = 1;
-goto cleanup;
+return 1;
 }
 
 if (uid)
 *uid = pw->pw_uid;
 ret = 0;
 
- cleanup:
 return ret;
 }
 
@@ -852,7 +850,7 @@ virGetGroupIDByName(const char *name, gid_t *gid, bool 
missing_ok)
 
 while ((rc = getgrnam_r(name, , strbuf, strbuflen, )) == ERANGE) {
 if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
-goto cleanup;
+return ret;
 }
 
 if (!gr) {
@@ -864,16 +862,13 @@ virGetGroupIDByName(const char *name, gid_t *gid, bool 
missing_ok)
  name, g_strerror(rc));
 }
 
-ret = 1;
-goto cleanup;
+return 1;
 }
 
 if (gid)
 *gid = gr->gr_gid;
-ret = 0;
 
- cleanup:
-return ret;
+return 0;
 }
 
 /* Try to match a group id based on `group`. The default behavior is to parse
@@ -981,18 +976,17 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 
 for (i = 0; i < ret; i++) {
 if ((*list)[i] == gid)
-goto cleanup;
+return ret;
 }
 if (VIR_APPEND_ELEMENT(*list, i, gid) < 0) {
 ret = -1;
 VIR_FREE(*list);
-goto cleanup;
+return ret;
 } else {
 ret = i;
 }
 }
 
- cleanup:
 return ret;
 }
 
@@ -1409,18 +1403,17 @@ virSetDeviceUnprivSGIO(const char *path,
 if (!virFileExists(sysfs_path)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("unpriv_sgio is not supported by this kernel"));
-goto cleanup;
+return ret;
 }
 
 val = g_strdup_printf("%d", unpriv_sgio);
 
 if ((rc = virFileWriteStr(sysfs_path, val, 0)) < 0) {
 virReportSystemError(-rc, _("failed to set %s"), sysfs_path);
-goto cleanup;
+return ret;
 }
 
 ret = 0;
- cleanup:
 return ret;
 }
 
@@ -1440,11 +1433,11 @@ virGetDeviceUnprivSGIO(const char *path,
 if (!virFileExists(sysfs_path)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("unpriv_sgio is not supported by this kernel"));
-goto cleanup;
+return ret;
 }
 
 if (virFileReadAll(sysfs_path, 1024, ) < 0)
-goto cleanup;
+return ret;
 
 if ((tmp = strchr(buf, '\n')))
 *tmp = '\0';
@@ -1452,12 +1445,10 @@ virGetDeviceUnprivSGIO(const char *path,
 if (virStrToLong_i(buf, NULL, 10, unpriv_sgio) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse value of %s"), sysfs_path);
-goto cleanup;
+return ret;
 }
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -1474,7 +1465,6 @@ virGetDeviceUnprivSGIO(const char *path,
 int
 virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
 {
-int rc = -1;
 uid_t theuid;
 gid_t thegid;
 g_autofree char *tmp_label = NULL;
@@ -1490,7 +1480,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, 
gid_t *gidPtr)
 virReportError(VIR_ERR_INVALID_ARG,
_("Failed to parse uid and gid from '%s'"),
label);
-goto cleanup;
+return -1;
 }
 *sep = '\0';
 owner = tmp_label;
@@ -1501,17 +1491,14 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, 
gid_t *gidPtr)
  */
 if (virGetUserID(owner, ) < 0 ||
 virGetGroupID(group, ) < 0)
-goto cleanup;
+return -1;
 
 if (uidPtr)
 *uidPtr = theuid;
 if (gidPtr)
 *gidPtr = thegid;
 
-rc = 0;
-
- cleanup:
-return rc;
+return 0;
 }
 
 static time_t selfLastChanged;
-- 
2.29.0

[PATCH 23/30] util: remove error label

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virsysinfo.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virsysinfo.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
index 36317c97eb..45a950c85a 100644
--- a/src/util/virsysinfo.c
+++ b/src/util/virsysinfo.c
@@ -411,7 +411,7 @@ virSysinfoParseARMProcessor(const char *base, 
virSysinfoDefPtr ret)
 cur = strchr(base, ':') + 1;
 
 if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0)
-goto error;
+return -1;
 processor = >processor[ret->nprocessor - 1];
 
 virSkipSpaces();
@@ -425,9 +425,6 @@ virSysinfoParseARMProcessor(const char *base, 
virSysinfoDefPtr ret)
 }
 
 return 0;
-
- error:
- return -1;
 }
 
 /* virSysinfoRead for ARMv7
-- 
2.29.0

[PATCH 22/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/viruri.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/viruri.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/viruri.c b/src/util/viruri.c
index 704e5b2132..d49821451e 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -241,10 +241,9 @@ virURIFormat(virURIPtr uri)
 ret = (char *)xmlSaveUri();
 if (!ret) {
 virReportOOMError();
-goto cleanup;
+return ret;
 }
 
- cleanup:
 return ret;
 }
 
-- 
2.29.0

[PATCH 12/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virnetdevbandwidth.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virnetdevbandwidth.c | 44 ---
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index c8eb5cfc79..364b39e3c1 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -102,7 +102,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname,
bool create_new)
 {
 int ret = -1;
-char *filter_id = NULL;
+g_autofree char *filter_id = NULL;
 virCommandPtr cmd = NULL;
 unsigned char ifmac[VIR_MAC_BUFLEN];
 char *mac[2] = {NULL, NULL};
@@ -157,7 +157,6 @@ virNetDevBandwidthManipulateFilter(const char *ifname,
  cleanup:
 VIR_FREE(mac[1]);
 VIR_FREE(mac[0]);
-VIR_FREE(filter_id);
 virCommandFree(cmd);
 return ret;
 }
@@ -195,9 +194,9 @@ virNetDevBandwidthSet(const char *ifname,
 int ret = -1;
 virNetDevBandwidthRatePtr rx = NULL, tx = NULL; /* From domain POV */
 virCommandPtr cmd = NULL;
-char *average = NULL;
-char *peak = NULL;
-char *burst = NULL;
+g_autofree char *average = NULL;
+g_autofree char *peak = NULL;
+g_autofree char *burst = NULL;
 
 if (!bandwidth) {
 /* nothing to be enabled */
@@ -385,9 +384,6 @@ virNetDevBandwidthSet(const char *ifname,
 
  cleanup:
 virCommandFree(cmd);
-VIR_FREE(average);
-VIR_FREE(peak);
-VIR_FREE(burst);
 return ret;
 }
 
@@ -533,10 +529,10 @@ virNetDevBandwidthPlug(const char *brname,
 {
 int ret = -1;
 virCommandPtr cmd = NULL;
-char *class_id = NULL;
-char *qdisc_id = NULL;
-char *floor = NULL;
-char *ceil = NULL;
+g_autofree char *class_id = NULL;
+g_autofree char *qdisc_id = NULL;
+g_autofree char *floor = NULL;
+g_autofree char *ceil = NULL;
 char ifmacStr[VIR_MAC_STRING_BUFLEN];
 
 if (id <= 2) {
@@ -586,10 +582,6 @@ virNetDevBandwidthPlug(const char *brname,
 ret = 0;
 
  cleanup:
-VIR_FREE(ceil);
-VIR_FREE(floor);
-VIR_FREE(qdisc_id);
-VIR_FREE(class_id);
 virCommandFree(cmd);
 return ret;
 }
@@ -610,8 +602,8 @@ virNetDevBandwidthUnplug(const char *brname,
 int ret = -1;
 int cmd_ret = 0;
 virCommandPtr cmd = NULL;
-char *class_id = NULL;
-char *qdisc_id = NULL;
+g_autofree char *class_id = NULL;
+g_autofree char *qdisc_id = NULL;
 
 if (id <= 2) {
 virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID %d"), id);
@@ -645,8 +637,6 @@ virNetDevBandwidthUnplug(const char *brname,
 ret = 0;
 
  cleanup:
-VIR_FREE(qdisc_id);
-VIR_FREE(class_id);
 virCommandFree(cmd);
 return ret;
 }
@@ -673,9 +663,9 @@ virNetDevBandwidthUpdateRate(const char *ifname,
 {
 int ret = -1;
 virCommandPtr cmd = NULL;
-char *class_id = NULL;
-char *rate = NULL;
-char *ceil = NULL;
+g_autofree char *class_id = NULL;
+g_autofree char *rate = NULL;
+g_autofree char *ceil = NULL;
 
 class_id = g_strdup_printf("1:%x", id);
 rate = g_strdup_printf("%llukbps", new_rate);
@@ -696,9 +686,6 @@ virNetDevBandwidthUpdateRate(const char *ifname,
 
  cleanup:
 virCommandFree(cmd);
-VIR_FREE(class_id);
-VIR_FREE(rate);
-VIR_FREE(ceil);
 return ret;
 }
 
@@ -725,7 +712,7 @@ virNetDevBandwidthUpdateFilter(const char *ifname,
unsigned int id)
 {
 int ret = -1;
-char *class_id = NULL;
+g_autofree char *class_id = NULL;
 
 class_id = g_strdup_printf("1:%x", id);
 
@@ -733,8 +720,7 @@ virNetDevBandwidthUpdateFilter(const char *ifname,
class_id, true, true) < 0)
 goto cleanup;
 
-ret = 0;
+return 0;
  cleanup:
-VIR_FREE(class_id);
 return ret;
 }
-- 
2.29.0

[PATCH 16/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virhostcpu.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virhostcpu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c531d65f86..4f6c3390ce 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
   int *nparams)
 {
 const char *sysctl_name;
-long *cpu_times;
+g_autofree long *cpu_times = NULL;
 struct clockinfo clkinfo;
 size_t i, j, cpu_times_size, clkinfo_size;
 int cpu_times_num, offset, hz, stathz, ret = -1;
@@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
 ret = 0;
 
  cleanup:
-VIR_FREE(cpu_times);
-
 return ret;
 }
 
-- 
2.29.0

[PATCH 13/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virmacmap.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virmacmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index f9047d0fb1..e68742de10 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -129,7 +129,7 @@ static int
 virMacMapLoadFile(virMacMapPtr mgr,
   const char *file)
 {
-char *map_str = NULL;
+g_autofree char *map_str = NULL;
 virJSONValuePtr map = NULL;
 int map_str_len = 0;
 size_t i;
@@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
 
 ret = 0;
  cleanup:
-VIR_FREE(map_str);
 virJSONValueFree(map);
 return ret;
 }
-- 
2.29.0

[PATCH 18/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virdnsmasq.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virdnsmasq.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index 9030f9218a..5f477c976d 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -164,7 +164,7 @@ addnhostsWrite(const char *path,
dnsmasqAddnHost *hosts,
unsigned int nhosts)
 {
-char *tmp;
+g_autofree char *tmp = NULL;
 FILE *f;
 bool istmp = true;
 size_t i, j;
@@ -230,8 +230,6 @@ addnhostsWrite(const char *path,
 }
 
  cleanup:
-VIR_FREE(tmp);
-
 return rc;
 }
 
@@ -364,7 +362,7 @@ hostsfileWrite(const char *path,
dnsmasqDhcpHost *hosts,
unsigned int nhosts)
 {
-char *tmp;
+g_autofree char *tmp = NULL;
 FILE *f;
 bool istmp = true;
 size_t i;
@@ -408,8 +406,6 @@ hostsfileWrite(const char *path,
 }
 
  cleanup:
-VIR_FREE(tmp);
-
 return rc;
 }
 
@@ -686,7 +682,7 @@ static int
 dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path)
 {
 int ret = -1;
-char *buf = NULL;
+g_autofree char *buf = NULL;
 
 if (virFileReadAll(path, 1024 * 1024, ) < 0)
 goto cleanup;
@@ -694,7 +690,6 @@ dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char 
*path)
 ret = dnsmasqCapsSetFromBuffer(caps, buf);
 
  cleanup:
-VIR_FREE(buf);
 return ret;
 }
 
@@ -704,7 +699,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force)
 int ret = -1;
 struct stat sb;
 virCommandPtr cmd = NULL;
-char *help = NULL, *version = NULL, *complete = NULL;
+g_autofree char *help = NULL;
+g_autofree char *version = NULL;
+g_autofree char *complete = NULL;
 
 if (!caps || caps->noRefresh)
 return 0;
@@ -749,9 +746,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force)
 
  cleanup:
 virCommandFree(cmd);
-VIR_FREE(help);
-VIR_FREE(version);
-VIR_FREE(complete);
 return ret;
 }
 
-- 
2.29.0

[PATCH 11/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virresctrl.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virresctrl.c | 25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index d3087b98c1..1c2d175295 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 {
 int ret = -1;
 int rv = -1;
-char *featurestr = NULL;
+g_autofree char *featurestr = NULL;
 char **features = NULL;
 size_t nfeatures = 0;
 virResctrlInfoMongrpPtr info_monitor = NULL;
@@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 
 ret = 0;
  cleanup:
-VIR_FREE(featurestr);
 g_strfreev(features);
 VIR_FREE(info_monitor);
 return ret;
@@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 const char *groupname,
 virResctrlAllocPtr *alloc)
 {
-char *schemata = NULL;
+g_autofree char *schemata = NULL;
 int rv = virFileReadValueString(,
 SYSFS_RESCTRL_PATH "/%s/schemata",
 groupname);
@@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
 goto error;
 
-VIR_FREE(schemata);
 return 0;
 
  error:
-VIR_FREE(schemata);
 virObjectUnref(*alloc);
 *alloc = NULL;
 return -1;
@@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
   virResctrlAllocPtr alloc,
   const char *machinename)
 {
-char *schemata_path = NULL;
-char *alloc_str = NULL;
+g_autofree char *schemata_path = NULL;
+g_autofree char *alloc_str = NULL;
 int ret = -1;
 int lockfd = -1;
 
@@ -2403,8 +2400,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
 ret = 0;
  cleanup:
 virResctrlUnlock(lockfd);
-VIR_FREE(alloc_str);
-VIR_FREE(schemata_path);
 return ret;
 }
 
@@ -2413,8 +2408,8 @@ static int
 virResctrlAddPID(const char *path,
  pid_t pid)
 {
-char *tasks = NULL;
-char *pidstr = NULL;
+g_autofree char *tasks = NULL;
+g_autofree char *pidstr = NULL;
 int ret = 0;
 
 if (!path) {
@@ -2436,8 +2431,6 @@ virResctrlAddPID(const char *path,
 
 ret = 0;
  cleanup:
-VIR_FREE(tasks);
-VIR_FREE(pidstr);
 return ret;
 }
 
@@ -2657,8 +2650,8 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
 size_t i = 0;
 unsigned long long val = 0;
 g_autoptr(DIR) dirp = NULL;
-char *datapath = NULL;
-char *filepath = NULL;
+g_autofree char *datapath = NULL;
+g_autofree char *filepath = NULL;
 struct dirent *ent = NULL;
 virResctrlMonitorStatsPtr stat = NULL;
 
@@ -2737,8 +2730,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
 
 ret = 0;
  cleanup:
-VIR_FREE(datapath);
-VIR_FREE(filepath);
 virResctrlMonitorStatsFree(stat);
 return ret;
 }
-- 
2.29.0

[PATCH 20/30] util: remove cleanup label

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virvhba.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virvhba.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/util/virvhba.c b/src/util/virvhba.c
index a80145b8fd..e0a79344cc 100644
--- a/src/util/virvhba.c
+++ b/src/util/virvhba.c
@@ -121,10 +121,10 @@ virVHBAGetConfig(const char *sysfs_prefix,
  sysfs_prefix ? sysfs_prefix : 
SYSFS_FC_HOST_PATH, host, entry);
 
 if (!virFileExists(sysfs_path))
-goto cleanup;
+return result;
 
 if (virFileReadAll(sysfs_path, 1024, ) < 0)
-goto cleanup;
+return result;
 
 if ((p = strchr(buf, '\n')))
 *p = '\0';
@@ -136,7 +136,6 @@ virVHBAGetConfig(const char *sysfs_prefix,
 
 result = g_strdup(p);
 
- cleanup:
 return result;
 }
 
@@ -206,15 +205,13 @@ virVHBAFindVportHost(const char *sysfs_prefix)
 if ((strlen(max_vports) >= strlen(vports)) ||
 ((strlen(max_vports) == strlen(vports)) &&
  strcmp(max_vports, vports) > 0)) {
-ret = g_strdup(entry->d_name);
-goto cleanup;
+return g_strdup(entry->d_name);
 }
 
 VIR_FREE(max_vports);
 VIR_FREE(vports);
 }
 
- cleanup:
 return ret;
 }
 
@@ -248,7 +245,7 @@ virVHBAManageVport(const int parent_host,
 default:
 virReportError(VIR_ERR_OPERATION_INVALID,
_("Invalid vport operation (%d)"), operation);
-goto cleanup;
+return ret;
 }
 
 operation_path = g_strdup_printf("%s/host%d/%s", SYSFS_FC_HOST_PATH,
@@ -264,7 +261,7 @@ virVHBAManageVport(const int parent_host,
_("vport operation '%s' is not supported "
  "for host%d"),
operation_file, parent_host);
-goto cleanup;
+return ret;
 }
 }
 
@@ -284,7 +281,6 @@ virVHBAManageVport(const int parent_host,
"vport create/delete failed"),
  vport_name, operation_path);
 
- cleanup:
 return ret;
 }
 
@@ -315,12 +311,11 @@ vhbaReadCompareWWN(const char *prefix,
 path = g_strdup_printf("%s/%s/%s", prefix, d_name, f_name);
 
 if (!virFileExists(path)) {
-ret = 0;
-goto cleanup;
+return 0;
 }
 
 if (virFileReadAll(path, 1024, ) < 0)
-goto cleanup;
+return ret;
 
 if ((p = strchr(buf, '\n')))
 *p = '\0';
@@ -334,8 +329,6 @@ vhbaReadCompareWWN(const char *prefix,
 else
 ret = 1;
 
- cleanup:
-
 return ret;
 }
 
@@ -418,7 +411,7 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
 
 if ((rc = vhbaReadCompareWWN(prefix, entry->d_name,
  "fabric_name", fabric_wwn)) < 0)
-goto cleanup;
+return ret;
 
 if (rc == 0)
 continue;
@@ -427,7 +420,6 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
 break;
 }
 
- cleanup:
 return ret;
 }
 
-- 
2.29.0

[PATCH 28/30] util: remove cleanup labels

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virnetdevbandwidth.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virnetdevbandwidth.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 364b39e3c1..e222bcd6a2 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -711,16 +711,13 @@ virNetDevBandwidthUpdateFilter(const char *ifname,
const virMacAddr *ifmac_ptr,
unsigned int id)
 {
-int ret = -1;
 g_autofree char *class_id = NULL;
 
 class_id = g_strdup_printf("1:%x", id);
 
 if (virNetDevBandwidthManipulateFilter(ifname, ifmac_ptr, id,
class_id, true, true) < 0)
-goto cleanup;
+return -1;
 
 return 0;
- cleanup:
-return ret;
 }
-- 
2.29.0

[PATCH 10/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virrotatingfile.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virrotatingfile.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
index 9f1ef17c3e..6d8076e7c4 100644
--- a/src/util/virrotatingfile.c
+++ b/src/util/virrotatingfile.c
@@ -362,8 +362,8 @@ static int
 virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 {
 size_t i;
-char *nextpath = NULL;
-char *thispath = NULL;
+g_autofree char *nextpath = NULL;
+g_autofree char *thispath = NULL;
 int ret = -1;
 
 VIR_DEBUG("Rollover %s", file->basepath);
@@ -403,8 +403,6 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
 
 ret = 0;
  cleanup:
-VIR_FREE(nextpath);
-VIR_FREE(thispath);
 return ret;
 }
 
-- 
2.29.0

[PATCH 07/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virstorageencryption.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virstorageencryption.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c
index 399c6926bd..ed29b4cfa5 100644
--- a/src/util/virstorageencryption.c
+++ b/src/util/virstorageencryption.c
@@ -142,7 +142,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt,
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 virStorageEncryptionSecretPtr ret;
-char *type_str = NULL;
+g_autofree char *type_str = NULL;
 
 ret = g_new0(virStorageEncryptionSecret, 1);
 
@@ -164,12 +164,9 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt,
 if (virSecretLookupParseSecret(node, >seclookupdef) < 0)
 goto cleanup;
 
-VIR_FREE(type_str);
-
 return ret;
 
  cleanup:
-VIR_FREE(type_str);
 virStorageEncryptionSecretFree(ret);
 return NULL;
 }
@@ -180,7 +177,7 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr info_node,
 virStorageEncryptionInfoDefPtr info)
 {
 int ret = -1;
-char *size_str = NULL;
+g_autofree char *size_str = NULL;
 
 if (!(info->cipher_name = virXMLPropString(info_node, "name"))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -208,7 +205,6 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr info_node,
 ret = 0;
 
  cleanup:
-VIR_FREE(size_str);
 return ret;
 }
 
@@ -237,7 +233,7 @@ virStorageEncryptionParseNode(xmlNodePtr node,
 xmlNodePtr *nodes = NULL;
 virStorageEncryptionPtr encdef = NULL;
 virStorageEncryptionPtr ret = NULL;
-char *format_str = NULL;
+g_autofree char *format_str = NULL;
 int n;
 size_t i;
 
@@ -297,7 +293,6 @@ virStorageEncryptionParseNode(xmlNodePtr node,
 ret = g_steal_pointer();
 
  cleanup:
-VIR_FREE(format_str);
 VIR_FREE(nodes);
 virStorageEncryptionFree(encdef);
 
-- 
2.29.0

[PATCH 02/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virvhba.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virvhba.c | 35 ---
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/src/util/virvhba.c b/src/util/virvhba.c
index a4e88024d1..a80145b8fd 100644
--- a/src/util/virvhba.c
+++ b/src/util/virvhba.c
@@ -49,7 +49,7 @@ bool
 virVHBAPathExists(const char *sysfs_prefix,
   int host)
 {
-char *sysfs_path = NULL;
+g_autofree char *sysfs_path = NULL;
 bool ret = false;
 
 sysfs_path = g_strdup_printf("%s/host%d",
@@ -58,7 +58,6 @@ virVHBAPathExists(const char *sysfs_prefix,
 if (virFileExists(sysfs_path))
 ret = true;
 
-VIR_FREE(sysfs_path);
 return ret;
 }
 
@@ -79,8 +78,8 @@ bool
 virVHBAIsVportCapable(const char *sysfs_prefix,
   int host)
 {
-char *scsi_host_path = NULL;
-char *fc_host_path = NULL;
+g_autofree char *scsi_host_path = NULL;
+g_autofree char *fc_host_path = NULL;
 bool ret = false;
 
 fc_host_path = g_strdup_printf("%s/host%d/%s",
@@ -94,8 +93,6 @@ virVHBAIsVportCapable(const char *sysfs_prefix,
 if (virFileExists(fc_host_path) || virFileExists(scsi_host_path))
 ret = true;
 
-VIR_FREE(fc_host_path);
-VIR_FREE(scsi_host_path);
 return ret;
 }
 
@@ -115,9 +112,9 @@ virVHBAGetConfig(const char *sysfs_prefix,
  int host,
  const char *entry)
 {
-char *sysfs_path = NULL;
+g_autofree char *sysfs_path = NULL;
 char *p = NULL;
-char *buf = NULL;
+g_autofree char *buf = NULL;
 char *result = NULL;
 
 sysfs_path = g_strdup_printf("%s/host%d/%s",
@@ -140,8 +137,6 @@ virVHBAGetConfig(const char *sysfs_prefix,
 result = g_strdup(p);
 
  cleanup:
-VIR_FREE(sysfs_path);
-VIR_FREE(buf);
 return result;
 }
 
@@ -160,8 +155,8 @@ virVHBAFindVportHost(const char *sysfs_prefix)
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
 g_autoptr(DIR) dir = NULL;
 struct dirent *entry = NULL;
-char *max_vports = NULL;
-char *vports = NULL;
+g_autofree char *max_vports = NULL;
+g_autofree char *vports = NULL;
 char *state = NULL;
 char *ret = NULL;
 
@@ -220,8 +215,6 @@ virVHBAFindVportHost(const char *sysfs_prefix)
 }
 
  cleanup:
-VIR_FREE(max_vports);
-VIR_FREE(vports);
 return ret;
 }
 
@@ -241,7 +234,8 @@ virVHBAManageVport(const int parent_host,
int operation)
 {
 int ret = -1;
-char *operation_path = NULL, *vport_name = NULL;
+g_autofree char *operation_path = NULL;
+g_autofree char *vport_name = NULL;
 const char *operation_file = NULL;
 
 switch (operation) {
@@ -291,8 +285,6 @@ virVHBAManageVport(const int parent_host,
  vport_name, operation_path);
 
  cleanup:
-VIR_FREE(vport_name);
-VIR_FREE(operation_path);
 return ret;
 }
 
@@ -315,8 +307,8 @@ vhbaReadCompareWWN(const char *prefix,
const char *f_name,
const char *wwn)
 {
-char *path;
-char *buf = NULL;
+g_autofree char *path = NULL;
+g_autofree char *buf = NULL;
 char *p;
 int ret = -1;
 
@@ -343,8 +335,6 @@ vhbaReadCompareWWN(const char *prefix,
 ret = 1;
 
  cleanup:
-VIR_FREE(path);
-VIR_FREE(buf);
 
 return ret;
 }
@@ -407,7 +397,7 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
 struct dirent *entry = NULL;
 g_autoptr(DIR) dir = NULL;
-char *vport_create_path = NULL;
+g_autofree char *vport_create_path = NULL;
 char *ret = NULL;
 
 if (virDirOpen(, prefix) < 0)
@@ -438,7 +428,6 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
 }
 
  cleanup:
-VIR_FREE(vport_create_path);
 return ret;
 }
 
-- 
2.29.0

[PATCH 05/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virsysinfo.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virsysinfo.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
index 217f842a37..36317c97eb 100644
--- a/src/util/virsysinfo.c
+++ b/src/util/virsysinfo.c
@@ -393,7 +393,7 @@ virSysinfoParseARMProcessor(const char *base, 
virSysinfoDefPtr ret)
 const char *cur;
 char *eol, *tmp_base;
 virSysinfoProcessorDefPtr processor;
-char *processor_type = NULL;
+g_autofree char *processor_type = NULL;
 
 if (!(tmp_base = strstr(base, "model name")) &&
 !(tmp_base = strstr(base, "Processor")))
@@ -424,12 +424,10 @@ virSysinfoParseARMProcessor(const char *base, 
virSysinfoDefPtr ret)
 base = cur;
 }
 
-VIR_FREE(processor_type);
 return 0;
 
  error:
-VIR_FREE(processor_type);
-return -1;
+ return -1;
 }
 
 /* virSysinfoRead for ARMv7
@@ -532,9 +530,9 @@ static int
 virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret)
 {
 const char *tmp_base;
-char *manufacturer = NULL;
-char *procline = NULL;
-char *ncpu = NULL;
+g_autofree char *manufacturer = NULL;
+g_autofree char *procline = NULL;
+g_autofree char *ncpu = NULL;
 int result = -1;
 virSysinfoProcessorDefPtr processor;
 
@@ -593,9 +591,6 @@ virSysinfoParseS390Processor(const char *base, 
virSysinfoDefPtr ret)
 result = 0;
 
  error:
-VIR_FREE(manufacturer);
-VIR_FREE(procline);
-VIR_FREE(ncpu);
 return result;
 }
 
-- 
2.29.0

[PATCH 03/30] util: convert pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

- src/util/virutil.c

Signed-off-by: Barrett Schonefeld 
---
 src/util/virutil.c | 33 ++---
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index a0cd0f1bcd..d4b864d5cb 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -598,7 +598,7 @@ char *virGetUserRuntimeDirectory(void)
 static int
 virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell, 
bool quiet)
 {
-char *strbuf;
+g_autofree char *strbuf = NULL;
 struct passwd pwbuf;
 struct passwd *pw = NULL;
 long val = sysconf(_SC_GETPW_R_SIZE_MAX);
@@ -668,13 +668,12 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char 
**dir, char **shell, bo
 if (shell)
 VIR_FREE(*shell);
 }
-VIR_FREE(strbuf);
 return ret;
 }
 
 static char *virGetGroupEnt(gid_t gid)
 {
-char *strbuf;
+g_autofree char *strbuf = NULL;
 char *ret;
 struct group grbuf;
 struct group *gr = NULL;
@@ -717,7 +716,6 @@ static char *virGetGroupEnt(gid_t gid)
 }
 
 ret = g_strdup(gr->gr_name);
-VIR_FREE(strbuf);
 return ret;
 }
 
@@ -759,7 +757,7 @@ char *virGetGroupName(gid_t gid)
 static int
 virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok)
 {
-char *strbuf = NULL;
+g_autofree char *strbuf = NULL;
 struct passwd pwbuf;
 struct passwd *pw = NULL;
 long val = sysconf(_SC_GETPW_R_SIZE_MAX);
@@ -796,8 +794,6 @@ virGetUserIDByName(const char *name, uid_t *uid, bool 
missing_ok)
 ret = 0;
 
  cleanup:
-VIR_FREE(strbuf);
-
 return ret;
 }
 
@@ -840,7 +836,7 @@ virGetUserID(const char *user, uid_t *uid)
 static int
 virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok)
 {
-char *strbuf = NULL;
+g_autofree char *strbuf = NULL;
 struct group grbuf;
 struct group *gr = NULL;
 long val = sysconf(_SC_GETGR_R_SIZE_MAX);
@@ -877,8 +873,6 @@ virGetGroupIDByName(const char *name, gid_t *gid, bool 
missing_ok)
 ret = 0;
 
  cleanup:
-VIR_FREE(strbuf);
-
 return ret;
 }
 
@@ -949,7 +943,7 @@ int
 virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 {
 int ret = 0;
-char *user = NULL;
+g_autofree char *user = NULL;
 gid_t primary;
 
 *list = NULL;
@@ -999,7 +993,6 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 }
 
  cleanup:
-VIR_FREE(user);
 return ret;
 }
 
@@ -1405,8 +1398,8 @@ virSetDeviceUnprivSGIO(const char *path,
const char *sysfs_dir,
int unpriv_sgio)
 {
-char *sysfs_path = NULL;
-char *val = NULL;
+g_autofree char *sysfs_path = NULL;
+g_autofree char *val = NULL;
 int ret = -1;
 int rc;
 
@@ -1428,8 +1421,6 @@ virSetDeviceUnprivSGIO(const char *path,
 
 ret = 0;
  cleanup:
-VIR_FREE(sysfs_path);
-VIR_FREE(val);
 return ret;
 }
 
@@ -1438,8 +1429,8 @@ virGetDeviceUnprivSGIO(const char *path,
const char *sysfs_dir,
int *unpriv_sgio)
 {
-char *sysfs_path = NULL;
-char *buf = NULL;
+g_autofree char *sysfs_path = NULL;
+g_autofree char *buf = NULL;
 char *tmp = NULL;
 int ret = -1;
 
@@ -1466,8 +1457,6 @@ virGetDeviceUnprivSGIO(const char *path,
 
 ret = 0;
  cleanup:
-VIR_FREE(sysfs_path);
-VIR_FREE(buf);
 return ret;
 }
 
@@ -1488,7 +1477,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, 
gid_t *gidPtr)
 int rc = -1;
 uid_t theuid;
 gid_t thegid;
-char *tmp_label = NULL;
+g_autofree char *tmp_label = NULL;
 char *sep = NULL;
 char *owner = NULL;
 char *group = NULL;
@@ -1522,8 +1511,6 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, 
gid_t *gidPtr)
 rc = 0;
 
  cleanup:
-VIR_FREE(tmp_label);
-
 return rc;
 }
 
-- 
2.29.0

[PATCH] virsh: Added attach-disk support for network disk

[Ryan Gahagan]

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport

Added documentation to virsh.rst specifying usage.

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst |  31 ++---
 tools/virsh-domain.c| 135 +++-
 2 files changed, 145 insertions(+), 21 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 1ae6d1a0d4..36c868a3e6 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4538,14 +4538,18 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
-*source* is path for the files and devices. *target* controls the bus or
-device under which the disk is exposed to the guest OS. It indicates the
-"logical" device name; the optional *targetbus* attribute specifies the type
+*source* is path for the files and devices unless *--source-protocol*
+is specified, in which case *source* is the name of a network disk.
+*target* controls the bus or device under which the disk is exposed
+to the guest OS. It indicates the "logical" device name;
+the optional *targetbus* attribute specifies the type
 of disk device to emulate; possible values are driver specific, with typical
 values being *ide*, *scsi*, *virtio*, *xen*, *usb*, *sata*, or *sd*, if
 omitted, the bus type is inferred from the style of the device name (e.g.  a
@@ -4563,7 +4567,7 @@ within the existing virtual cdrom or floppy device; 
consider using
 ``update-device`` for this usage instead.
 *alias* can set user supplied alias.
 *mode* can specify the two specific mode *readonly* or *shareable*.
-*sourcetype* can indicate the type of source (block|file)
+*sourcetype* can indicate the type of source (block|file|network)
 *cache* can be one of "default", "none", "writethrough", "writeback",
 "directsync" or "unsafe".
 *io* controls specific policies on I/O; QEMU guests support "threads",
@@ -4579,6 +4583,19 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their 
cssid set to 0xfe.
 *multifunction* indicates specified pci address is a multifunction pci device
 address.
 
+There is also support for using a network disk. As specified, the user can
+provide a *--source-protocol* in which case the *source* parameter will
+be interpreted as the source name. *--source-protocol* must be provided
+if the user intends to provide a network disk or host information.
+Host information can be provided using the tags
+*--source-host-name*, *--source-host-transport*, and *--source-host-socket*,
+which respectively denote the name of the host, the host's transport method,
+and the socket that the host uses. *--source-host-socket* and
+*--source-host-name* cannot both be provided, and the user must provide a
+*--source-host-transport* if they want to provide a *--source-host-socket*.
+The *--source-host-name* parameter supports host:port syntax
+if the user wants to provide a port as well.
+
 If *--print-xml* is specified, then the XML of the disk that would be attached
 is printed instead.
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c999458d72..1303676c33 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -222,7 +222,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 {.name = "source",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
- .help = N_("source of disk device")
+ .help = N_("source of disk device or name of network disk")
 },
 {.name = "target",
  .type = VSH_OT_DATA,
@@ -268,7 +268,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 },
 {.name = "sourcetype",
  .type = VSH_OT_STRING,
- .help = N_("type of source (block|file)")
+ .help = N_("type of source (block|file|network)")
 },
 {.name = "serial",
  .type = VSH_OT_STRING,
@@ -298,6 +298,22 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .type = VSH_OT_BOOL,
  .help = N_("print XML document rather than attach the disk")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("

[PATCH] util: convert char pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

additional conversions to the GLib API in src/util per issue #11.

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11

Signed-off-by: Barrett Schonefeld 
---
 src/util/vircgroupv1.c   |  3 +-
 src/util/virdnsmasq.c| 43 -
 src/util/virfile.c   |  9 ++---
 src/util/virhostcpu.c|  4 +-
 src/util/virlockspace.c  |  6 +--
 src/util/virlog.c| 12 ++
 src/util/virmacmap.c |  3 +-
 src/util/virnetdevbandwidth.c| 48 ---
 src/util/virresctrl.c| 25 
 src/util/virrotatingfile.c   | 11 ++
 src/util/virscsihost.c   | 25 +---
 src/util/virsecret.c | 14 +++
 src/util/virstorageencryption.c  | 19 +++--
 src/util/virstoragefilebackend.c |  4 +-
 src/util/virsysinfo.c| 18 +++--
 src/util/viruri.c|  7 +---
 src/util/virutil.c   | 66 +++-
 src/util/virvhba.c   | 57 ++-
 src/util/virxml.c|  7 +---
 19 files changed, 130 insertions(+), 251 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index 9030f9218a..93bc4a129f 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -164,7 +164,7 @@ addnhostsWrite(const char *path,
dnsmasqAddnHost *hosts,
unsigned int nhosts)
 {
-char *tmp;
+g_autofree char *tmp = NULL;
 FILE *f;
 bool istmp = true;
 size_t i, j;
@@ -180,7 +180,7 @@ addnhostsWrite(const char *path,
 istmp = false;
 if (!(f = fopen(path, "w"))) {
 rc = -errno;
-goto cleanup;
+return rc;
 }
 }
 
@@ -192,7 +192,7 @@ addnhostsWrite(const char *path,
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return rc;
 }
 
 for (j = 0; j < hosts[i].nhostnames; j++) {
@@ -203,7 +203,7 @@ addnhostsWrite(const char *path,
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return rc;
 }
 }
 
@@ -214,24 +214,21 @@ addnhostsWrite(const char *path,
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return rc;
 }
 }
 
 if (VIR_FCLOSE(f) == EOF) {
 rc = -errno;
-goto cleanup;
+return rc;
 }
 
 if (istmp && rename(tmp, path) < 0) {
 rc = -errno;
 unlink(tmp);
-goto cleanup;
+return rc;
 }
 
- cleanup:
-VIR_FREE(tmp);
-
 return rc;
 }
 
@@ -364,7 +361,7 @@ hostsfileWrite(const char *path,
dnsmasqDhcpHost *hosts,
unsigned int nhosts)
 {
-char *tmp;
+g_autofree char *tmp = NULL;
 FILE *f;
 bool istmp = true;
 size_t i;
@@ -380,7 +377,7 @@ hostsfileWrite(const char *path,
 istmp = false;
 if (!(f = fopen(path, "w"))) {
 rc = -errno;
-goto cleanup;
+return rc;
 }
 }
 
@@ -392,24 +389,21 @@ hostsfileWrite(const char *path,
 if (istmp)
 unlink(tmp);
 
-goto cleanup;
+return rc;
 }
 }
 
 if (VIR_FCLOSE(f) == EOF) {
 rc = -errno;
-goto cleanup;
+return rc;
 }
 
 if (istmp && rename(tmp, path) < 0) {
 rc = -errno;
 unlink(tmp);
-goto cleanup;
+return rc;
 }
 
- cleanup:
-VIR_FREE(tmp);
-
 return rc;
 }
 
@@ -686,15 +680,13 @@ static int
 dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path)
 {
 int ret = -1;
-char *buf = NULL;
+g_autofree char *buf = NULL;
 
 if (virFileReadAll(path, 1024 * 1024, ) < 0)
-goto cleanup;
+return ret;
 
 ret = dnsmasqCapsSetFromBuffer(caps, buf);
 
- cleanup:
-VIR_FREE(buf);
 return ret;
 }
 
@@ -704,7 +696,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force)
 int ret = -1;
 struct stat sb;
 virCommandPtr cmd = NULL;
-char *help = NULL, *version = NULL, *complete = NULL;
+g_autofree char *help = NULL;
+g_autofree char *version = NULL;
+g_autofree char *complete = NULL;
 
 if (!caps || caps->noRefresh)
 

Re: [PATCH] virsh: Added attach-disk support for network disk

[Ryan Gahagan]

> > +
> >  /* Make XML of disk */
> >  virBufferAsprintf(, " >isFile ? "file" : "block");
>
> I didn't notice this previously. You obviously need " if defining a network disk.

We're confused on what the exact semantics of the disk type we should use
is, particularly if the driver or stype field has unexpected values.. We've
drafted up some example code to show our interpretation of when "network"
should be used over "file" or "block", but it is by no means final and we
want your opinion on it. Here's what we've got:

typedef enum {
VIRSH_SOURCE_TYPE_FILE,
VIRSH_SOURCE_TYPE_BLOCK,
VIRSH_SOURCE_TYPE_NETWORK, VIRSH_SOURCE_TYPE_UNKNOWN,
} virshAttachDiskSourceType;

...
virshAttachDiskSourceType disk_type = VIRSH_SOURCE_TYPE_UNKNOWN;
if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap")))
{ /* Disk type declared to be file */ disk_type = VIRSH_SOURCE_TYPE_FILE; }
else { if (source && !stat(source, )) { /* Found a file at path source,
test file type */ if (S_ISREG(st.st_mode)) disk_type =
VIRSH_SOURCE_TYPE_FILE; else if (S_ISBLK(st.st_mode)) disk_type =
VIRSH_SOURCE_TYPE_BLOCK; else disk_type = VIRSH_SOURCE_TYPE_NETWORK; } else
{ /* Either file not found or not specified, default network */ disk_type =
VIRSH_SOURCE_TYPE_NETWORK; } } } else if (STREQ(stype, "file")) { disk_type
= VIRSH_SOURCE_TYPE_FILE; } else if (STREQ(stype, "block")) { disk_type =
VIRSH_SOURCE_TYPE_BLOCK; } else if (STREQ(stype, "network")) { disk_type =
VIRSH_SOURCE_TYPE_NETWORK; } else { vshError(ctl, _("Unknown source type:
'%s'"), stype); goto cleanup; }

Let us know if this looks right or what you think should be used instead.

> > +/* Using a local disk; source is file or dev */
> > +virBufferAsprintf(, " %s='%s'",
> +isFile ? "file" : "dev", source);
>
> This is still misaligned.
>
> > +virBufferAddLit(, "/>\n");
> > +}
> > +}
> > +
> >  virBufferAsprintf(, " >  if (targetbus)
> >  virBufferAsprintf(, " bus='%s'", targetbus);
>
> Unfortunately this function is very old and would need to be refactored,
> the XML formatting is definitely not to our modern standards.

We're not entirely sure what you mean by this. What part of the code looks
misaligned, and what function would need to be refactored? Your example
output at least looks aligned properly:

> $ ./build/libvirt/gcc/tools/virsh attach-disk upstream-bj /tmp/ble vda
--source-host-name test:1234 --source-protocol test --print-xml
> 
> 
> 
>   
>   
> 

What specifically would you want to be changed?

On Mon, Nov 16, 2020 at 6:58 AM Peter Krempa  wrote:

> On Fri, Nov 13, 2020 at 12:52:32 -0600, Ryan Gahagan wrote:
> > Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
> > Added in support for the following parameters in attach-disk:
> > --source-protocol
> > --source-host-name
> > --source-host-socket
> > --source-host-transport
> >
> > Added documentation to virsh.rst specifying usage.
> >
> > Signed-off-by: Ryan Gahagan 
> > ---
> >  docs/manpages/virsh.rst | 26 ++---
> >  tools/virsh-domain.c| 85 ++---
> >  2 files changed, 100 insertions(+), 11 deletions(-)
> >
> > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> > index bfd26e3120..a4d70e9211 100644
> > --- a/docs/manpages/virsh.rst
> > +++ b/docs/manpages/virsh.rst
> > @@ -4500,14 +4500,18 @@ attach-disk
> >[--current]] | [--persistent]] [--targetbus bus]
> >[--driver driver] [--subdriver subdriver] [--iothread iothread]
> >[--cache cache] [--io io] [--type type] [--alias alias]
> > -  [--mode mode] [--sourcetype sourcetype] [--serial serial]
> > -  [--wwn wwn] [--rawio] [--address address] [--multifunction]
> > -  [--print-xml]
> > +  [--mode mode] [--sourcetype sourcetype]
> > +  [--source-protocol protocol] [--source-host-name hostname:port]
> > +  [--source-host-transport transport] [--source-host-socket socket]
> > +  [--serial serial] [--wwn wwn] [--rawio] [--address address]
> > +  [--multifunction] [--print-xml]
> >
> >  Attach a new disk device to the domain.
> > -*source* is path for the files and devices. *target* controls the bus or
> > -device under which the disk is exposed to the guest OS. It indicates the
> > -"logical" device name; the optional *targetbus* attribute specifies the
> type
> > 

[PATCH] virsh: Added attach-disk support for network disk

[Ryan Gahagan]

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport

Added documentation to virsh.rst specifying usage.

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst | 26 ++---
 tools/virsh-domain.c| 85 ++---
 2 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..a4d70e9211 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4500,14 +4500,18 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
-*source* is path for the files and devices. *target* controls the bus or
-device under which the disk is exposed to the guest OS. It indicates the
-"logical" device name; the optional *targetbus* attribute specifies the type
+*source* is path for the files and devices unless *--source-protocol*
+is specified, in which case *source* is the name of a network disk.
+*target* controls the bus or device under which the disk is exposed
+to the guest OS. It indicates the "logical" device name;
+the optional *targetbus* attribute specifies the type
 of disk device to emulate; possible values are driver specific, with typical
 values being *ide*, *scsi*, *virtio*, *xen*, *usb*, *sata*, or *sd*, if
 omitted, the bus type is inferred from the style of the device name (e.g.  a
@@ -4541,6 +4545,16 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their 
cssid set to 0xfe.
 *multifunction* indicates specified pci address is a multifunction pci device
 address.
 
+There is also support for using a network disk. As specified, the user can
+provide a *--source-protocol* in which case the *source* parameter will
+be interpreted as the source name. *--source-protocol* must be provided
+if the user also wishes to provide host information.
+Host information can be provided using any of the tags
+*--source-host-name*, *--source-host-transport*, and *--source-host-socket*,
+which respectively denote the name of the host, the host's transport method,
+and the socket that the host uses. The *--source-host-name* parameter
+supports host:port syntax if the user wants to provide a port as well.
+
 If *--print-xml* is specified, then the XML of the disk that would be attached
 is printed instead.
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..4c43da7a2c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -222,7 +222,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 {.name = "source",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
- .help = N_("source of disk device")
+ .help = N_("source of disk device or name of network disk")
 },
 {.name = "target",
  .type = VSH_OT_DATA,
@@ -298,6 +298,22 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .type = VSH_OT_BOOL,
  .help = N_("print XML document rather than attach the disk")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name = "source-host-transport",
+ .type = VSH_OT_STRING,
+ .help = N_("host transport for source of disk device")
+},
+{.name = "source-host-socket",
+ .type = VSH_OT_STRING,
+ .help = N_("host socket for source of disk device")
+},
 VIRSH_COMMON_OPT_DOMAIN_PERSISTENT,
 VIRSH_COMMON_OPT_DOMAIN_CONFIG,
 VIRSH_COMMON_OPT_DOMAIN_LIVE,
@@ -567,6 +583,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 *iothread = NULL, *cache = NULL, *io = NULL,
 *serial = NULL, *straddr = NULL, *wwn = NULL,
 *targetbus = NULL, *alias = NULL;
+const char *source_protocol = NULL;
+const char *host_name = NULL;
+const char *host_transport = NULL;
+const char *host_socket = NULL;
+char *host_name_copy = NULL;
+char *host_port = NULL;
 struct DiskAddress diskAddr;
 bool isFile

[PATCH] Added attach-disk parameters for network disk support

[Ryan Gahagan]

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-name
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport

Allowed for multiple hosts to be added to a single source.
Multiple hosts can be defined by providing multiple instances of
--source-host-name, followed by optional transport and socket
parameters.
Using a single host does not require a host name.

Added documentation to virsh.rst specifying usage.

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst |  24 +++-
 tools/virsh-domain.c| 125 +---
 2 files changed, 138 insertions(+), 11 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..60e5f5ebe4 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4500,9 +4500,11 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype] [--source-name name]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
 *source* is path for the files and devices. *target* controls the bus or
@@ -4541,6 +4543,22 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their 
cssid set to 0xfe.
 *multifunction* indicates specified pci address is a multifunction pci device
 address.
 
+If *--source-protocol* or *--source-name* is specified, then the parameters
+will be inserted into the XML that is generated for the source.
+If any of *--source-host-name*, *--source-host-transport*, or
+*--source-host-socket* are specified, then a  tag
+will be generated under the  tag containing whichever
+parameters were provided. If needed, the user can provide multiple hosts
+by providing each host with a *--source-host-name*. Each host will
+receive the host parameters which come between it and the next instance
+of *--source-host-name* or between it and the end of the command.
+If a user tries to provide multiple of the same host parameter
+for any single host, only the first one will be generated as
+part of the XML output.
+
+--source-host-name me --source-host-transport t1 --source-host-transport t2 
--source-host-transport t3 --soure-host-name you
+
+
 If *--print-xml* is specified, then the XML of the disk that would be attached
 is printed instead.
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..609189e398 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -224,6 +224,26 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
  .help = N_("source of disk device")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+},
+{.name = "source-name",
+ .type = VSH_OT_STRING,
+ .help = N_("name of disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name = "source-host-transport",
+ .type = VSH_OT_STRING,
+ .help = N_("host transport for source of disk device")
+},
+{.name = "source-host-socket",
+ .type = VSH_OT_STRING,
+ .help = N_("host socket for source of disk device")
+},
 {.name = "target",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
@@ -558,15 +578,68 @@ static int str2DiskAddress(const char *str, struct 
DiskAddress *diskAddr)
 return -1;
 }
 
+static void attachDiskHostGen(virBufferPtr buf, const vshCmd *cmd)
+{
+// Can be multiple hosts so we have to scan
+// the cmd options to find all the host params
+// opts;
+char *host_name = NULL, *host_port = NULL;
+int close_tag = 0, seen_socket = 0, seen_transport = 0;
+
+while (candidate) {
+// Iterate candidates to find each host-name
+if (STREQ(candidate->def->name, "source-host-name")) {
+// After the first host-name, we need to terminate
+// the \n");
+else
+close_tag = 1;
+
+host_name = candidate->data;
+host_port = strchr(host_name, ':');
+
+if (!host_port) {
+// If port isn't provided, only print name
+virBufferAsprintf(buf, "def

[PATCH] Added attach-disk parameters for network disk support

[Ryan Gahagan]

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-name
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport

Allowed for multiple hosts to be added to a single source.
Multiple hosts can be defined by providing multiple instances of
--source-host-name, followed by optional transport and socket
parameters.
Using a single host does not require a host name.

Added documentation to virsh.rst specifying usage.

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst |  24 +++-
 tools/virsh-domain.c| 125 +---
 2 files changed, 138 insertions(+), 11 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..60e5f5ebe4 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4500,9 +4500,11 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype] [--source-name name]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
 *source* is path for the files and devices. *target* controls the bus or
@@ -4541,6 +4543,22 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their 
cssid set to 0xfe.
 *multifunction* indicates specified pci address is a multifunction pci device
 address.
 
+If *--source-protocol* or *--source-name* is specified, then the parameters
+will be inserted into the XML that is generated for the source.
+If any of *--source-host-name*, *--source-host-transport*, or
+*--source-host-socket* are specified, then a  tag
+will be generated under the  tag containing whichever
+parameters were provided. If needed, the user can provide multiple hosts
+by providing each host with a *--source-host-name*. Each host will
+receive the host parameters which come between it and the next instance
+of *--source-host-name* or between it and the end of the command.
+If a user tries to provide multiple of the same host parameter
+for any single host, only the first one will be generated as
+part of the XML output.
+
+--source-host-name me --source-host-transport t1 --source-host-transport t2 
--source-host-transport t3 --soure-host-name you
+
+
 If *--print-xml* is specified, then the XML of the disk that would be attached
 is printed instead.
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..469665f0f0 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -224,6 +224,26 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
  .help = N_("source of disk device")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+},
+{.name = "source-name",
+ .type = VSH_OT_STRING,
+ .help = N_("name of disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name = "source-host-transport",
+ .type = VSH_OT_STRING,
+ .help = N_("host transport for source of disk device")
+},
+{.name = "source-host-socket",
+ .type = VSH_OT_STRING,
+ .help = N_("host socket for source of disk device")
+},
 {.name = "target",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
@@ -558,15 +578,68 @@ static int str2DiskAddress(const char *str, struct 
DiskAddress *diskAddr)
 return -1;
 }
 
+static void attachDiskHostGen(virBufferPtr buf, const vshCmd *cmd)
+{
+// Can be multiple hosts so we have to scan
+// the cmd options to find all the host params
+// opts;
+char *host_name = NULL, *host_port = NULL;
+int close_tag = 0, seen_socket, seen_transport;
+
+while (candidate) {
+// Iterate candidates to find each host-name
+if (STREQ(candidate->def->name, "source-host-name")) {
+// After the first host-name, we need to terminate
+// the \n");
+else
+close_tag = 1;
+
+host_name = candidate->data;
+host_port = strchr(host_name, ':');
+
+if (!host_port) {
+// If port isn't provided, only print name
+virBufferAsprintf(buf, "def->name, 
"so

[PATCH 1/8] Added a few attach-disk parameters

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 76 +++-
 1 file changed, 68 insertions(+), 8 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..16227085cc 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -224,6 +224,26 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
  .help = N_("source of disk device")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+}
+{.name = "source-name",
+ .type = VSH_OT_STRING,
+ .help = N_("name of disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name = "source-host-transport",
+ .type = VSH_OT_STRING,
+ .help = N_("host transport for source of disk device")
+},
+{.name = "source-host-socket",
+ .type = VSH_OT_STRING,
+ .help = N_("host socket for source of disk device")
+},
 {.name = "target",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
@@ -562,11 +582,13 @@ static bool
 cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom = NULL;
-const char *source = NULL, *target = NULL, *driver = NULL,
-*subdriver = NULL, *type = NULL, *mode = NULL,
-*iothread = NULL, *cache = NULL, *io = NULL,
-*serial = NULL, *straddr = NULL, *wwn = NULL,
-*targetbus = NULL, *alias = NULL;
+const char *source = NULL, *source_name = NULL, *source_protocol = NULL,
+*target = NULL, *driver = NULL, *subdriver = NULL, *type = 
NULL,
+*mode = NULL, *iothread = NULL, *cache = NULL,
+*io = NULL, *serial = NULL, *straddr = NULL,
+*wwn = NULL, *targetbus = NULL, *alias = NULL,
+*host_name = NULL, *host_transport = NULL,
+*host_port = NULL, *host_socket = NULL;
 struct DiskAddress diskAddr;
 bool isFile = false, functionReturn = false;
 int ret;
@@ -591,6 +613,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_AFFECT_LIVE;
 
 if (vshCommandOptStringReq(ctl, cmd, "source", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-protocol", _protocol) 
< 0 ||
 vshCommandOptStringReq(ctl, cmd, "target", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "driver", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "subdriver", ) < 0 ||
@@ -604,7 +628,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
-vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
+vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-transport", 
_transport) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) < 
0)
 goto cleanup;
 
 if (!stype) {
@@ -659,9 +686,42 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 virBufferAddLit(, "/>\n");
 }
 
-if (source)
-virBufferAsprintf(, "\n",
+if (source || source_protocol || source_name ||
+host_name || host_transport || host_socket) {
+virBufferAsprintf(, "\n\n\n");
+} else {
+virBufferAsprintf(" />\n");
+}
+}
+
 virBufferAsprintf(, "

[PATCH 6/8] Bump - rerun CI HTTP 500 err

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9d00cb8b21..ae9a2b1101 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -711,7 +711,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 virBufferAsprintf(, " name='%s' port='%s'", host_name, 
host_port + 1);
 }
 }
-
 if (host_transport)
 virBufferAsprintf(, " transport='%s'", host_transport);
 if (host_socket)
-- 
2.29.0

[PATCH 3/8] Fixed code style bug for virBufferAddLit

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 810e55fa53..5862993464 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -688,7 +688,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 
 if (source || source_protocol || source_name ||
 host_name || host_transport || host_socket) {
-virBufferAsprintf(, "\n\n\n\n");
+virBufferAddLit(, " />\n\n");
 } else {
-virBufferAsprintf(" />\n");
+virBufferAddLit(, " />\n");
 }
 }
 
-- 
2.29.0

[PATCH 2/8] Fixed compile error where was missing

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 16227085cc..810e55fa53 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -707,7 +707,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 if (!host_port)
 virBufferAsprintf(" name='%s'", host_name);
 else {
-host_name[host_port - host_name] = '\0';
+host_name[(int)(host_port - host_name)] = '\0';
 virBufferAsprintf(" name='%s' port='%s'", host_name, 
host_port + 1);
 }
 }
-- 
2.29.0

[PATCH 4/8] Fixed error of writing to ROM in host_name

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5862993464..cda0531a37 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -587,8 +587,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 *mode = NULL, *iothread = NULL, *cache = NULL,
 *io = NULL, *serial = NULL, *straddr = NULL,
 *wwn = NULL, *targetbus = NULL, *alias = NULL,
-*host_name = NULL, *host_transport = NULL,
-*host_port = NULL, *host_socket = NULL;
+*host_transport = NULL, *host_port = NULL, *host_socket = NULL;
 struct DiskAddress diskAddr;
 bool isFile = false, functionReturn = false;
 int ret;
@@ -596,6 +595,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 const char *stype = NULL;
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 char *xml = NULL;
+char *host_name = NULL;
 struct stat st;
 bool current = vshCommandOptBool(cmd, "current");
 bool config = vshCommandOptBool(cmd, "config");
-- 
2.29.0

[PATCH 8/8] Added new params to attach-disk docs

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..83134ba571 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4500,9 +4500,11 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype] [--source-name name]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
 *source* is path for the files and devices. *target* controls the bus or
-- 
2.29.0

[PATCH 7/8] Re-wrote attach-disk to support multiple hosts

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 70 ++--
 1 file changed, 55 insertions(+), 15 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ae9a2b1101..64c7c454bd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -578,6 +578,49 @@ static int str2DiskAddress(const char *str, struct 
DiskAddress *diskAddr)
 return -1;
 }
 
+static void attachDiskHostGen(virBufferPtr buf, const vshCmd *cmd)
+{
+// Can be multiple hosts so we have to scan
+// the cmd options to find all the host params
+// opts;
+char *host_name = NULL, *host_port = NULL;
+int closeTag = 0;
+
+while (candidate) {
+// Iterate candidates to find each host-name
+if (STREQ(candidate->def->name, "source-host-name")) {
+// After the first host-name, we need to terminate
+// the \n");
+else
+closeTag = 1;
+
+host_name = candidate->data;
+host_port = strchr(host_name, ':');
+
+if (!host_port) {
+// If port isn't provided, only print name
+virBufferAsprintf(buf, "def->name, "source-host-socket")) {
+virBufferAsprintf(buf, " socket='%s'", candidate->data);
+} else if (STREQ(candidate->def->name, "source-host-transport")) {
+virBufferAsprintf(buf, " transport='%s'", candidate->data);
+}
+
+candidate = candidate->next;
+}
+// Close final \n");
+}
+
 static bool
 cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 {
@@ -587,7 +630,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 *mode = NULL, *iothread = NULL, *cache = NULL,
 *io = NULL, *serial = NULL, *straddr = NULL,
 *wwn = NULL, *targetbus = NULL, *alias = NULL,
-*host_transport = NULL, *host_port = NULL, *host_socket = NULL;
+*host_transport = NULL, *host_name = NULL, *host_socket = NULL;
 struct DiskAddress diskAddr;
 bool isFile = false, functionReturn = false;
 int ret;
@@ -595,7 +638,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 const char *stype = NULL;
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 char *xml = NULL;
-char *host_name = NULL;
 struct stat st;
 bool current = vshCommandOptBool(cmd, "current");
 bool config = vshCommandOptBool(cmd, "config");
@@ -698,27 +740,25 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 if (source_name)
 virBufferAsprintf(, " name='%s'", source_name);
 
-if (host_name || host_transport || host_socket) {
+if (!(host_name || host_transport || host_socket)) {
+// Close source if no host is provided
+virBufferAddLit(, "/>\n");
+} else if (!host_name) {
+// If no host name is provided but there is a host,
+// we have a single host with params
 virBufferAddLit(, ">\n\n\n");
+virBufferAddLit(, "/>\n\n");
 } else {
-virBufferAddLit(, " />\n");
+// May have multiple hosts, use helper method
+virBufferAddLit(, ">\n");
+attachDiskHostGen(, cmd);
+virBufferAddLit(, "\n");
 }
 }
 
-- 
2.29.0

[PATCH 5/8] Fixed error where char** not being promoted to const

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cda0531a37..9d00cb8b21 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -227,7 +227,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 {.name = "source-protocol",
  .type = VSH_OT_STRING,
  .help = N_("protocol used by disk device source")
-}
+},
 {.name = "source-name",
  .type = VSH_OT_STRING,
  .help = N_("name of disk device source")
@@ -629,7 +629,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
-vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-name", (const char **) 
_name) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "source-host-transport", 
_transport) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) < 
0)
 goto cleanup;
-- 
2.29.0

[PATCH] util: convert char pointers to use g_autofree

[Ryan Gahagan]

From: Barrett Schonefeld 

additional conversions to the GLib API in src/util per issue #11.

files updated are:
- src/util/vircgroupv1.c
- src/util/virhostcpu.c
- src/util/virlockspace.c
- src/util/virmacmap.c
- src/util/virresctrl.c
- src/util/virsysinfo.c

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11

Signed-off-by: bschoney 
Signed-off-by: Barrett Schonefeld 
---
 src/util/vircgroupv1.c  |  3 +--
 src/util/virhostcpu.c   |  4 +---
 src/util/virlockspace.c |  6 ++
 src/util/virmacmap.c|  3 +--
 src/util/virresctrl.c   | 25 -
 src/util/virsysinfo.c   |  9 +++--
 6 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c531d65f86..4f6c3390ce 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
   int *nparams)
 {
 const char *sysctl_name;
-long *cpu_times;
+g_autofree long *cpu_times = NULL;
 struct clockinfo clkinfo;
 size_t i, j, cpu_times_size, clkinfo_size;
 int cpu_times_num, offset, hz, stathz, ret = -1;
@@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
 ret = 0;
 
  cleanup:
-VIR_FREE(cpu_times);
-
 return ret;
 }
 
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index b90e13f506..71d5dfb83e 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
@@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index f9047d0fb1..e68742de10 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -129,7 +129,7 @@ static int
 virMacMapLoadFile(virMacMapPtr mgr,
   const char *file)
 {
-char *map_str = NULL;
+g_autofree char *map_str = NULL;
 virJSONValuePtr map = NULL;
 int map_str_len = 0;
 size_t i;
@@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
 
 ret = 0;
  cleanup:
-VIR_FREE(map_str);
 virJSONValueFree(map);
 return ret;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index d3087b98c1..1c2d175295 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 {
 int ret = -1;
 int rv = -1;
-char *featurestr = NULL;
+g_autofree char *featurestr = NULL;
 char **features = NULL;
 size_t nfeatures = 0;
 virResctrlInfoMongrpPtr info_monitor = NULL;
@@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 
 ret = 0;
  cleanup:
-VIR_FREE(featurestr);
 g_strfreev(features);
 VIR_FREE(info_monitor);
 return ret;
@@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 const char *groupname,
 virResctrlAllocPtr *alloc)
 {
-char *schemata = NULL;
+g_autofree char *schemata = NULL;
 int rv = virFileReadValueString(,
 SYSFS_RESCTRL_PATH "/%s/schemata",
 groupname);
@@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
 goto error;
 
-VIR_FREE(schemata);
 return 0;
 
  error:
-VIR_FREE(schemata);
 virObjectUnref(*alloc);
 *alloc = NULL;
 return -1;
@@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
   virResctrlAllocPtr alloc,
   const char 

[PATCH 5/5] Fixed error where char** not being promoted to const

[Ryan Gahagan]

Signed-off-by: Ryan Gahagan 
---
 tools/virsh-domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 8abca1b65e..10ff7175a2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -227,7 +227,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 {.name = "source-protocol",
  .type = VSH_OT_STRING,
  .help = N_("protocol used by disk device source")
-}
+},
 {.name = "source-name",
  .type = VSH_OT_STRING,
  .help = N_("name of disk device source")
@@ -629,7 +629,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
-vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-name", (const char **) 
_name) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "source-host-transport", 
_transport) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) < 
0)
 goto cleanup;
-- 
2.29.0

[PATCH] convert char pointers to use g_autofree

[Ryan Gahagan]

From: bschoney 

additional conversions to the GLib API in src/util per issue #11.

Please let me know if there are additional changes I should make in the files 
updated so far.
I intend to submit work on additional files, and I want to be sure the changes 
so far are correct.

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11

Signed-off-by: bschoney 
---
 src/util/vircgroupv1.c  |  3 +--
 src/util/virhostcpu.c   |  4 +---
 src/util/virlockspace.c |  6 ++
 src/util/virmacmap.c|  3 +--
 src/util/virresctrl.c   | 25 -
 src/util/virsysinfo.c   |  9 +++--
 6 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c531d65f86..4f6c3390ce 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
   int *nparams)
 {
 const char *sysctl_name;
-long *cpu_times;
+g_autofree long *cpu_times = NULL;
 struct clockinfo clkinfo;
 size_t i, j, cpu_times_size, clkinfo_size;
 int cpu_times_num, offset, hz, stathz, ret = -1;
@@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
 ret = 0;
 
  cleanup:
-VIR_FREE(cpu_times);
-
 return ret;
 }
 
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index 2731d46dfc..c88a24be36 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
@@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index 2d203e72af..70b148acac 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -129,7 +129,7 @@ static int
 virMacMapLoadFile(virMacMapPtr mgr,
   const char *file)
 {
-char *map_str = NULL;
+g_autofree char *map_str = NULL;
 virJSONValuePtr map = NULL;
 int map_str_len = 0;
 size_t i;
@@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
 
 ret = 0;
  cleanup:
-VIR_FREE(map_str);
 virJSONValueFree(map);
 return ret;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index d3087b98c1..1c2d175295 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 {
 int ret = -1;
 int rv = -1;
-char *featurestr = NULL;
+g_autofree char *featurestr = NULL;
 char **features = NULL;
 size_t nfeatures = 0;
 virResctrlInfoMongrpPtr info_monitor = NULL;
@@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 
 ret = 0;
  cleanup:
-VIR_FREE(featurestr);
 g_strfreev(features);
 VIR_FREE(info_monitor);
 return ret;
@@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 const char *groupname,
 virResctrlAllocPtr *alloc)
 {
-char *schemata = NULL;
+g_autofree char *schemata = NULL;
 int rv = virFileReadValueString(,
 SYSFS_RESCTRL_PATH "/%s/schemata",
 groupname);
@@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
 goto error;
 
-VIR_FREE(schemata);
 return 0;
 
  error:
-VIR_FREE(schemata);
 virObjectUnref(*alloc);
 *alloc = NULL;
 return -1;
@@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
   virResctrlAllocPtr alloc,
   const char *machinename)
 {
- 

[PATCH] convert char pointers to use g_autofree

[Ryan Gahagan]

From: bschoney 

additional conversions to the GLib API in src/util per issue #11.

Please let me know if there are additional changes I should make in the files 
updated so far.
I intend to submit work on additional files, and I want to be sure the changes 
so far are correct.

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11

Signed-off-by: bschoney 
---
 src/util/vircgroupv1.c  |  3 +--
 src/util/virhostcpu.c   |  4 +---
 src/util/virlockspace.c |  6 ++
 src/util/virmacmap.c|  3 +--
 src/util/virresctrl.c   | 25 -
 src/util/virsysinfo.c   |  9 +++--
 6 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c531d65f86..4f6c3390ce 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
   int *nparams)
 {
 const char *sysctl_name;
-long *cpu_times;
+g_autofree long *cpu_times = NULL;
 struct clockinfo clkinfo;
 size_t i, j, cpu_times_size, clkinfo_size;
 int cpu_times_num, offset, hz, stathz, ret = -1;
@@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
 ret = 0;
 
  cleanup:
-VIR_FREE(cpu_times);
-
 return ret;
 }
 
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index 2731d46dfc..c88a24be36 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
@@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index 2d203e72af..70b148acac 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -129,7 +129,7 @@ static int
 virMacMapLoadFile(virMacMapPtr mgr,
   const char *file)
 {
-char *map_str = NULL;
+g_autofree char *map_str = NULL;
 virJSONValuePtr map = NULL;
 int map_str_len = 0;
 size_t i;
@@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
 
 ret = 0;
  cleanup:
-VIR_FREE(map_str);
 virJSONValueFree(map);
 return ret;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index d3087b98c1..1c2d175295 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 {
 int ret = -1;
 int rv = -1;
-char *featurestr = NULL;
+g_autofree char *featurestr = NULL;
 char **features = NULL;
 size_t nfeatures = 0;
 virResctrlInfoMongrpPtr info_monitor = NULL;
@@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 
 ret = 0;
  cleanup:
-VIR_FREE(featurestr);
 g_strfreev(features);
 VIR_FREE(info_monitor);
 return ret;
@@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 const char *groupname,
 virResctrlAllocPtr *alloc)
 {
-char *schemata = NULL;
+g_autofree char *schemata = NULL;
 int rv = virFileReadValueString(,
 SYSFS_RESCTRL_PATH "/%s/schemata",
 groupname);
@@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
 goto error;
 
-VIR_FREE(schemata);
 return 0;
 
  error:
-VIR_FREE(schemata);
 virObjectUnref(*alloc);
 *alloc = NULL;
 return -1;
@@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
   virResctrlAllocPtr alloc,
   const char *machinename)
 {
- 

[PATCH] convert char pointers to use g_autofree

[Ryan Gahagan]

From: bschoney 

additional conversions to the GLib API in src/util per issue #11.

Please let me know if there are additional changes I should make in the files 
updated so far.
I intend to submit work on additional files, and I want to be sure the changes 
so far are correct.

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11

Signed-off-by: bschoney 
---
 src/util/vircgroupv1.c  |  3 +--
 src/util/virhostcpu.c   |  4 +---
 src/util/virlockspace.c |  6 ++
 src/util/virmacmap.c|  3 +--
 src/util/virresctrl.c   | 25 -
 src/util/virsysinfo.c   |  9 +++--
 6 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
  unsigned long long *unevictable)
 {
 int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
 char *line = NULL;
 unsigned long long cacheVal = 0;
 unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
 ret = 0;
 
  cleanup:
-VIR_FREE(stat);
 return ret;
 }
 
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c531d65f86..4f6c3390ce 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
   int *nparams)
 {
 const char *sysctl_name;
-long *cpu_times;
+g_autofree long *cpu_times = NULL;
 struct clockinfo clkinfo;
 size_t i, j, cpu_times_size, clkinfo_size;
 int cpu_times_num, offset, hz, stathz, ret = -1;
@@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
 ret = 0;
 
  cleanup:
-VIR_FREE(cpu_times);
-
 return ret;
 }
 
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index b90e13f506..71d5dfb83e 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
@@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
const char *resname)
 {
 int ret = -1;
-char *respath = NULL;
+g_autofree char *respath = NULL;
 
 VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
 
@@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 
  cleanup:
 virMutexUnlock(>lock);
-VIR_FREE(respath);
 return ret;
 }
 
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index f9047d0fb1..e68742de10 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -129,7 +129,7 @@ static int
 virMacMapLoadFile(virMacMapPtr mgr,
   const char *file)
 {
-char *map_str = NULL;
+g_autofree char *map_str = NULL;
 virJSONValuePtr map = NULL;
 int map_str_len = 0;
 size_t i;
@@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
 
 ret = 0;
  cleanup:
-VIR_FREE(map_str);
 virJSONValueFree(map);
 return ret;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index d3087b98c1..1c2d175295 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 {
 int ret = -1;
 int rv = -1;
-char *featurestr = NULL;
+g_autofree char *featurestr = NULL;
 char **features = NULL;
 size_t nfeatures = 0;
 virResctrlInfoMongrpPtr info_monitor = NULL;
@@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
 
 ret = 0;
  cleanup:
-VIR_FREE(featurestr);
 g_strfreev(features);
 VIR_FREE(info_monitor);
 return ret;
@@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 const char *groupname,
 virResctrlAllocPtr *alloc)
 {
-char *schemata = NULL;
+g_autofree char *schemata = NULL;
 int rv = virFileReadValueString(,
 SYSFS_RESCTRL_PATH "/%s/schemata",
 groupname);
@@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl,
 if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
 goto error;
 
-VIR_FREE(schemata);
 return 0;
 
  error:
-VIR_FREE(schemata);
 virObjectUnref(*alloc);
 *alloc = NULL;
 return -1;
@@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
   virResctrlAllocPtr alloc,
   const char *machinename)
 {
- 

Re: Libvirt Open Source Contribution

[Ryan Gahagan]

We've also been having some troubles actually getting ninja and meson to
run properly. Our team has one member on MacOS, one on Ubuntu 18.04, and
one working on a remote server (Ubuntu again) without sudo privileges. We
want to be able to run ninja test to properly test and clean our code as we
make pull requests, but it's been very difficult to get set up.

The Ubuntu aptitude store has an outdated version of meson that doesn't
actually run properly, and the pip3 version is up to date but then the
build dependencies are left unresolved. These dependencies are also by
default not actually in the aptitude store either, nor can they easily be
mass installed via homebrew (to our knowledge). Even after manually
configuring aptitude to find the links to all the dependencies of the
project, manually installing meson and ninja, and installing the
dependencies, we are still left with an error that says "XDR is required
for remote driver". Most of our team cannot even reach this point, as any
of the earlier steps is not reproducible due to either the environment not
having the correct tooling or lacking sufficient administrator privilege to
execute them.

All of our code we've written thus far has relied entirely upon the CI to
build the project for us, which is not a particularly efficient workflow
due to the time it takes for CI to finish. How can we get ninja test (and
other build tools) to actually run on our machines? Do you have any
additional instructions that we may be able to use outside of the
CONTRIBUTING.rst file?

-Ryan Gahagan

On Sat, Nov 7, 2020 at 12:04 PM Barrett J Schonefeld 
wrote:

> Michal,
>
> We've struggled with the email system so far. You may have seen when we
> submitted a chain of 65 patches when we meant to send only the last commit.
> We tested this by sending it to ourselves first but must have made a
> mistake when submitting to libvir-list.
>
> We submitted an initial patch for issue-11, which was rejected because
> Ryan emailed the patch, but I was CCed. I am working on getting my computer
> set up to submit this patch myself.
>
> Additionally, we submitted a patch for issue-16 (from Ryan Gahagan), and
> we feel confident we did this correctly (even received a confirmation
> email). However, we have not seen the patch posted on the forum nor have we
> received any email that the patch was rejected. Is it possible it has not
> been posted because we left outstanding questions in the patch?
>
> Best regards,
> Barrett Schonefeld
>
> On Fri, Nov 6, 2020 at 2:03 AM Michal Prívozník 
> wrote:
>
>> On 11/5/20 10:04 PM, Barrett J Schonefeld wrote:
>> > Hey folks,
>> >
>> > We appreciate the feedback, and we've used this to complete some
>> initial
>> > work on issue 11.
>> >
>> > We started with small changes in src/util, and we submitted them via
>> > email (following these guidelines on submitting patches
>> > <https://libvirt.org/submitting-patches.html>) to ensure we are on the
>> > right path.
>> >
>> > We do not see the patch we submitted via email showing up in the
>> > threads, and we are wondering if we are submitting incorrectly or if
>> the
>> > patch goes through some approval process before appearing in the
>> threads.
>> >
>> > Best regards,
>> > Barrett Schonefeld
>>
>> So I checked the moderator's queue and there are no patches stuck, so
>> probably it is something else. Did you see any error when 'git
>> send-email'-ing the patches? When I was setting up my machine I used to
>> send patches just to me to verify send-email is working and only then I
>> tried to send patches to the list.
>>
>> Also, are you sure you (the sender) are subscribed to the list?
>> Generally, if you aren't then list won't forward your e-mails/patches.
>>
>> Michal
>>
>>