[PATCH 7/8] virsh: Add wrapper for virNWFilterFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk|  2 +-
 tools/virsh-completer-nwfilter.c |  3 ++-
 tools/virsh-nwfilter.c   | 22 +++---
 tools/virsh-util.c   | 11 +++
 tools/virsh-util.h   |  5 +
 5 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 6c230826bd..5daf5afcd0 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|NodeDevice|Secret|StoragePool|StorageVol)Free\b'
 \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|NodeDevice|NWFilter|Secret|StoragePool|StorageVol)Free\b'
 \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-completer-nwfilter.c b/tools/virsh-completer-nwfilter.c
index 859f72f6e9..9850dbba5d 100644
--- a/tools/virsh-completer-nwfilter.c
+++ b/tools/virsh-completer-nwfilter.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-nwfilter.h"
+#include "virsh-util.h"
 #include "viralloc.h"
 #include "virsh.h"
 #include "virstring.h"
@@ -56,7 +57,7 @@ virshNWFilterNameCompleter(vshControl *ctl,
 ret = g_steal_pointer();
 
 for (i = 0; i < nnwfilters; i++)
-virNWFilterFree(nwfilters[i]);
+virshNWFilterFree(nwfilters[i]);
 g_free(nwfilters);
 return ret;
 }
diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c
index 33164f623f..09ceaf6ec9 100644
--- a/tools/virsh-nwfilter.c
+++ b/tools/virsh-nwfilter.c
@@ -20,6 +20,7 @@
 
 #include 
 #include "virsh-nwfilter.h"
+#include "virsh-util.h"
 
 #include "internal.h"
 #include "viralloc.h"
@@ -91,7 +92,7 @@ static const vshCmdOptDef opts_nwfilter_define[] = {
 static bool
 cmdNWFilterDefine(vshControl *ctl, const vshCmd *cmd)
 {
-virNWFilterPtr nwfilter;
+g_autoptr(virshNWFilter) nwfilter = NULL;
 const char *from = NULL;
 bool ret = true;
 g_autofree char *buffer = NULL;
@@ -115,7 +116,6 @@ cmdNWFilterDefine(vshControl *ctl, const vshCmd *cmd)
 if (nwfilter != NULL) {
 vshPrintExtra(ctl, _("Network filter %s defined from %s\n"),
   virNWFilterGetName(nwfilter), from);
-virNWFilterFree(nwfilter);
 } else {
 vshError(ctl, _("Failed to define network filter from %s"), from);
 ret = false;
@@ -149,7 +149,7 @@ static const vshCmdOptDef opts_nwfilter_undefine[] = {
 static bool
 cmdNWFilterUndefine(vshControl *ctl, const vshCmd *cmd)
 {
-virNWFilterPtr nwfilter;
+g_autoptr(virshNWFilter) nwfilter = NULL;
 bool ret = true;
 const char *name;
 
@@ -163,7 +163,6 @@ cmdNWFilterUndefine(vshControl *ctl, const vshCmd *cmd)
 ret = false;
 }
 
-virNWFilterFree(nwfilter);
 return ret;
 }
 
@@ -193,7 +192,7 @@ static const vshCmdOptDef opts_nwfilter_dumpxml[] = {
 static bool
 cmdNWFilterDumpXML(vshControl *ctl, const vshCmd *cmd)
 {
-virNWFilterPtr nwfilter;
+g_autoptr(virshNWFilter) nwfilter = NULL;
 bool ret = true;
 g_autofree char *dump = NULL;
 
@@ -207,7 +206,6 @@ cmdNWFilterDumpXML(vshControl *ctl, const vshCmd *cmd)
 ret = false;
 }
 
-virNWFilterFree(nwfilter);
 return ret;
 }
 
@@ -239,8 +237,7 @@ virshNWFilterListFree(struct virshNWFilterList *list)
 
 if (list && list->filters) {
 for (i = 0; i < list->nfilters; i++) {
-if (list->filters[i])
-virNWFilterFree(list->filters[i]);
+virshNWFilterFree(list->filters[i]);
 }
 g_free(list->filters);
 }
@@ -418,8 +415,8 @@ static bool
 cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd)
 {
 bool ret = false;
-virNWFilterPtr nwfilter = NULL;
-virNWFilterPtr nwfilter_edited = NULL;
+g_autoptr(virshNWFilter) nwfilter = NULL;
+g_autoptr(virshNWFilter) nwfilter_edited = NULL;
 virshControl *priv = ctl->privData;
 
 nwfilter = virshCommandOptNWFilter(ctl, cmd, NULL);
@@ -445,11 +442,6 @@ cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd)
 ret = true;
 
  cleanup:
-if (nwfilter)
-virNWFilterFree(nwfilter);
-if (nwfilter_edited)
-virNWFilterFree(nwfilter_edited);
-
 return ret;
 }
 
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 5034f4773f..fc2839d8fc 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -318,6 +318,17 @@ virshNodeDeviceFree(virNodeDevicePtr device)
 }
 
 
+void
+virshNWFilterFree(virNWFilterPtr nwfilter)
+{
+if (!nwfilter)
+return;
+
+vshSaveLibvirtHelperError();
+virNWFilterFree(nwfilter); /* sc_prohibit_obj_free_apis_in_virsh */
+}
+

[PATCH 6/8] virsh: Add wrapper for virNodeDeviceFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk   |  2 +-
 tools/virsh-completer-nodedev.c |  3 +-
 tools/virsh-nodedev.c   | 49 ++---
 tools/virsh-util.c  | 11 
 tools/virsh-util.h  |  5 
 5 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 0c0d844d6c..6c230826bd 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|Secret|StoragePool|StorageVol)Free\b'
 \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|NodeDevice|Secret|StoragePool|StorageVol)Free\b'
 \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-completer-nodedev.c b/tools/virsh-completer-nodedev.c
index d595b687fd..d10bf2b78c 100644
--- a/tools/virsh-completer-nodedev.c
+++ b/tools/virsh-completer-nodedev.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-nodedev.h"
+#include "virsh-util.h"
 #include "conf/node_device_conf.h"
 #include "viralloc.h"
 #include "virsh-nodedev.h"
@@ -58,7 +59,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl,
 ret = g_steal_pointer();
 
 for (i = 0; i < ndevs; i++)
-virNodeDeviceFree(devs[i]);
+virshNodeDeviceFree(devs[i]);
 g_free(devs);
 return ret;
 }
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index f72359121f..c989a77ad2 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -20,6 +20,7 @@
 
 #include 
 #include "virsh-nodedev.h"
+#include "virsh-util.h"
 
 #include "internal.h"
 #include "viralloc.h"
@@ -55,7 +56,7 @@ static const vshCmdOptDef opts_node_device_create[] = {
 static bool
 cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd)
 {
-virNodeDevicePtr dev = NULL;
+g_autoptr(virshNodeDevice) dev = NULL;
 const char *from = NULL;
 g_autofree char *buffer = NULL;
 virshControl *priv = ctl->privData;
@@ -73,7 +74,6 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd)
 
 vshPrintExtra(ctl, _("Node device %s created from %s\n"),
   virNodeDeviceGetName(dev), from);
-virNodeDeviceFree(dev);
 return true;
 }
 
@@ -140,7 +140,7 @@ vshFindNodeDevice(vshControl *ctl, const char *value)
 static bool
 cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
 {
-virNodeDevice *dev = NULL;
+g_autoptr(virshNodeDevice) dev = NULL;
 bool ret = false;
 const char *device_value = NULL;
 
@@ -160,8 +160,6 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
 
 ret = true;
  cleanup:
-if (dev)
-virNodeDeviceFree(dev);
 return ret;
 }
 
@@ -207,8 +205,7 @@ virshNodeDeviceListFree(struct virshNodeDeviceList *list)
 
 if (list && list->devices) {
 for (i = 0; i < list->ndevices; i++) {
-if (list->devices[i])
-virNodeDeviceFree(list->devices[i]);
+virshNodeDeviceFree(list->devices[i]);
 }
 g_free(list->devices);
 }
@@ -327,8 +324,7 @@ virshNodeDeviceListCollect(vshControl *ctl,
 
  remove_entry:
 /* the device has to be removed as it failed one of the filters */
-virNodeDeviceFree(list->devices[i]);
-list->devices[i] = NULL;
+g_clear_pointer(>devices[i], virshNodeDeviceFree);
 deleted++;
 }
 
@@ -576,7 +572,7 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = {
 static bool
 cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
 {
-virNodeDevicePtr device = NULL;
+g_autoptr(virshNodeDevice) device = NULL;
 g_autofree char *xml = NULL;
 const char *device_value = NULL;
 bool ret = false;
@@ -596,8 +592,6 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
 
 ret = true;
  cleanup:
-if (device)
-virNodeDeviceFree(device);
 return ret;
 }
 
@@ -634,7 +628,7 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd)
 {
 const char *name = NULL;
 const char *driverName = NULL;
-virNodeDevicePtr device;
+g_autoptr(virshNodeDevice) device = NULL;
 bool ret = true;
 virshControl *priv = ctl->privData;
 
@@ -664,7 +658,6 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd)
 else
 vshError(ctl, _("Failed to detach device %s"), name);
 
-virNodeDeviceFree(device);
 return ret;
 }
 
@@ -696,7 +689,7 @@ static bool
 cmdNodeDeviceReAttach(vshControl *ctl, const vshCmd *cmd)
 {
 const char *name = NULL;
-virNodeDevicePtr device;
+g_autoptr(virshNodeDevice) device = NULL;
 bool ret = true;
 virshControl *priv = ctl->privData;
 

[PATCH 5/8] virsh: Add wrapper for virNetworkFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk   |  2 +-
 tools/virsh-completer-network.c |  8 ++--
 tools/virsh-network.c   | 75 ++---
 tools/virsh-util.c  | 11 +
 tools/virsh-util.h  |  5 +++
 5 files changed, 43 insertions(+), 58 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 2bdbd14c80..0c0d844d6c 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret|StoragePool|StorageVol)Free\b'
 \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|Secret|StoragePool|StorageVol)Free\b'
 \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c
index f9154f23a4..d498f59cb8 100644
--- a/tools/virsh-completer-network.c
+++ b/tools/virsh-completer-network.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-network.h"
+#include "virsh-util.h"
 #include "viralloc.h"
 #include "virsh-network.h"
 #include "virsh.h"
@@ -60,7 +61,7 @@ virshNetworkNameCompleter(vshControl *ctl,
 ret = g_steal_pointer();
 
 for (i = 0; i < nnets; i++)
-virNetworkFree(nets[i]);
+virshNetworkFree(nets[i]);
 g_free(nets);
 return ret;
 }
@@ -170,7 +171,7 @@ virshNetworkUUIDCompleter(vshControl *ctl,
 
  cleanup:
 for (i = 0; i < nnets; i++)
-virNetworkFree(nets[i]);
+virshNetworkFree(nets[i]);
 g_free(nets);
 return ret;
 }
@@ -183,7 +184,7 @@ virshNetworkDhcpMacCompleter(vshControl *ctl,
 {
 virshControl *priv = ctl->privData;
 virNetworkDHCPLeasePtr *leases = NULL;
-virNetworkPtr network = NULL;
+g_autoptr(virshNetwork) network = NULL;
 int nleases;
 size_t i = 0;
 char **ret = NULL;
@@ -215,6 +216,5 @@ virshNetworkDhcpMacCompleter(vshControl *ctl,
 virNetworkDHCPLeaseFree(leases[i]);
 VIR_FREE(leases);
 }
-virNetworkFree(network);
 return ret;
 }
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 1442210278..02b22bf912 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -20,6 +20,7 @@
 
 #include 
 #include "virsh-network.h"
+#include "virsh-util.h"
 
 #include "internal.h"
 #include "viralloc.h"
@@ -155,7 +156,7 @@ static const vshCmdOptDef opts_network_autostart[] = {
 static bool
 cmdNetworkAutostart(vshControl *ctl, const vshCmd *cmd)
 {
-virNetworkPtr network;
+g_autoptr(virshNetwork) network = NULL;
 const char *name;
 int autostart;
 
@@ -169,7 +170,6 @@ cmdNetworkAutostart(vshControl *ctl, const vshCmd *cmd)
 vshError(ctl, _("failed to mark network %s as autostarted"), name);
 else
 vshError(ctl, _("failed to unmark network %s as autostarted"), 
name);
-virNetworkFree(network);
 return false;
 }
 
@@ -178,7 +178,6 @@ cmdNetworkAutostart(vshControl *ctl, const vshCmd *cmd)
 else
 vshPrintExtra(ctl, _("Network %s unmarked as autostarted\n"), name);
 
-virNetworkFree(network);
 return true;
 }
 
@@ -207,7 +206,7 @@ static const vshCmdOptDef opts_network_create[] = {
 static bool
 cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd)
 {
-virNetworkPtr network;
+g_autoptr(virshNetwork) network = NULL;
 const char *from = NULL;
 g_autofree char *buffer = NULL;
 unsigned int flags = 0;
@@ -234,7 +233,6 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd)
 
 vshPrintExtra(ctl, _("Network %s created from %s\n"),
   virNetworkGetName(network), from);
-virNetworkFree(network);
 return true;
 }
 
@@ -264,7 +262,7 @@ static const vshCmdOptDef opts_network_define[] = {
 static bool
 cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd)
 {
-virNetworkPtr network;
+g_autoptr(virshNetwork) network = NULL;
 const char *from = NULL;
 g_autofree char *buffer = NULL;
 unsigned int flags = 0;
@@ -291,7 +289,6 @@ cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd)
 
 vshPrintExtra(ctl, _("Network %s defined from %s\n"),
   virNetworkGetName(network), from);
-virNetworkFree(network);
 return true;
 }
 
@@ -316,7 +313,7 @@ static const vshCmdOptDef opts_network_destroy[] = {
 static bool
 cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd)
 {
-virNetworkPtr network;
+g_autoptr(virshNetwork) network = NULL;
 bool ret = true;
 const char *name;
 
@@ -330,7 +327,6 @@ cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd)
 ret = false;
 }
 
-virNetworkFree(network);
 return ret;

[PATCH 4/8] virsh: Add wrapper for virStorageVolFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk  |  2 +-
 tools/virsh-completer-volume.c |  4 +-
 tools/virsh-domain.c   |  3 +-
 tools/virsh-util.c | 11 ++
 tools/virsh-util.h |  5 +++
 tools/virsh-volume.c   | 72 ++
 6 files changed, 40 insertions(+), 57 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 111d2109e8..2bdbd14c80 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret|StoragePool)Free\b' \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret|StoragePool|StorageVol)Free\b'
 \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-completer-volume.c b/tools/virsh-completer-volume.c
index 1d83643c69..ac3c472177 100644
--- a/tools/virsh-completer-volume.c
+++ b/tools/virsh-completer-volume.c
@@ -65,7 +65,7 @@ virshStorageVolNameCompleter(vshControl *ctl,
 
  cleanup:
 for (i = 0; i < nvols; i++)
-virStorageVolFree(vols[i]);
+virshStorageVolFree(vols[i]);
 g_free(vols);
 return ret;
 }
@@ -104,7 +104,7 @@ virshStorageVolKeyCompleter(vshControl *ctl,
 const char *key = virStorageVolGetKey(vols[j]);
 tmp[nvols] = g_strdup(key);
 nvols++;
-virStorageVolFree(vols[j]);
+virshStorageVolFree(vols[j]);
 }
 
 g_free(vols);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 461a5e19f6..b1943b3875 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3942,8 +3942,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 for (i = 0; i < nvols; i++) {
 VIR_FREE(vols[i].source);
 VIR_FREE(vols[i].target);
-if (vols[i].vol)
-virStorageVolFree(vols[i].vol);
+virshStorageVolFree(vols[i].vol);
 }
 VIR_FREE(vols);
 
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index d537501387..c680c5b3d4 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -318,6 +318,17 @@ virshStoragePoolFree(virStoragePoolPtr pool)
 }
 
 
+void
+virshStorageVolFree(virStorageVolPtr vol)
+{
+if (!vol)
+return;
+
+vshSaveLibvirtHelperError();
+virStorageVolFree(vol); /* sc_prohibit_obj_free_apis_in_virsh */
+}
+
+
 int
 virshDomainGetXMLFromDom(vshControl *ctl,
  virDomainPtr dom,
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 3ff6f16784..ce3462a865 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -69,6 +69,11 @@ void
 virshStoragePoolFree(virStoragePoolPtr pool);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshStoragePool, virshStoragePoolFree);
 
+typedef virStorageVol virshStorageVol;
+void
+virshStorageVolFree(virStorageVolPtr vol);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshStorageVol, virshStorageVolFree);
+
 int
 virshDomainState(vshControl *ctl,
  virDomainPtr dom,
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 6e8f7721a3..b896ebbbf9 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -153,8 +153,7 @@ virshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd,
 vshError(ctl,
  _("Requested volume '%s' is not in pool '%s'"),
  n, virStoragePoolGetName(pool));
-virStorageVolFree(vol);
-vol = NULL;
+g_clear_pointer(, virshStorageVolFree);
 }
 }
 }
@@ -230,7 +229,7 @@ static bool
 cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
 {
 g_autoptr(virshStoragePool) pool = NULL;
-virStorageVolPtr vol = NULL;
+g_autoptr(virshStorageVol) vol = NULL;
 g_autofree char *xml = NULL;
 bool printXML = vshCommandOptBool(cmd, "print-xml");
 const char *name, *capacityStr = NULL, *allocationStr = NULL, *format = 
NULL;
@@ -287,7 +286,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
 
 /* Convert the snapshot parameters into backingStore XML */
 if (snapshotStrVol) {
-virStorageVolPtr snapVol;
+g_autoptr(virshStorageVol) snapVol = NULL;
 g_autofree char *snapshotStrVolPath = NULL;
 /* Lookup snapshot backing volume.  Try the backing-vol
  *  parameter as a name */
@@ -330,7 +329,6 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
 }
 
 if ((snapshotStrVolPath = virStorageVolGetPath(snapVol)) == NULL) {
-virStorageVolFree(snapVol);
 goto cleanup;
 }
 
@@ -343,9 +341,6 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
   

[PATCH 2/8] virsh: Add wrapper for virInterfaceFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk |  4 +--
 tools/virsh-completer-interface.c |  3 +-
 tools/virsh-interface.c   | 55 +--
 tools/virsh-util.c| 11 +++
 tools/virsh-util.h|  5 +++
 5 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index cb54c8ba36..84cb895d86 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,10 +868,10 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   @prohibit='\bvir(Domain|DomainSnapshot|Secret)Free\b' \
+   @prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret)Free\b' \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
-   halt='avoid using virDomain(Snapshot)Free in virsh, use virsh-prefixed 
wrappers instead' \
+   halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
  $(_sc_search_regexp)
 
 https_sites = www.libvirt.org
diff --git a/tools/virsh-completer-interface.c 
b/tools/virsh-completer-interface.c
index 9a6603b27e..44ba35550a 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-interface.h"
+#include "virsh-util.h"
 #include "viralloc.h"
 #include "virsh.h"
 #include "virstring.h"
@@ -59,7 +60,7 @@ virshInterfaceStringHelper(vshControl *ctl,
 }
 
 for (i = 0; i < nifaces; i++)
-virInterfaceFree(ifaces[i]);
+virshInterfaceFree(ifaces[i]);
 g_free(ifaces);
 
 return g_steal_pointer();
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index f402119b68..0a8539da71 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -29,6 +29,7 @@
 
 #include 
 #include "virsh-interface.h"
+#include "virsh-util.h"
 
 #include 
 #include 
@@ -109,8 +110,8 @@ static bool
 cmdInterfaceEdit(vshControl *ctl, const vshCmd *cmd)
 {
 bool ret = false;
-virInterfacePtr iface = NULL;
-virInterfacePtr iface_edited = NULL;
+g_autoptr(virshInterface) iface = NULL;
+g_autoptr(virshInterface) iface_edited = NULL;
 unsigned int flags = VIR_INTERFACE_XML_INACTIVE;
 virshControl *priv = ctl->privData;
 
@@ -136,11 +137,6 @@ cmdInterfaceEdit(vshControl *ctl, const vshCmd *cmd)
 ret = true;
 
  cleanup:
-if (iface)
-virInterfaceFree(iface);
-if (iface_edited)
-virInterfaceFree(iface_edited);
-
 return ret;
 }
 
@@ -172,8 +168,7 @@ virshInterfaceListFree(struct virshInterfaceList *list)
 
 if (list && list->ifaces) {
 for (i = 0; i < list->nifaces; i++) {
-if (list->ifaces[i])
-virInterfaceFree(list->ifaces[i]);
+virshInterfaceFree(list->ifaces[i]);
 }
 g_free(list->ifaces);
 }
@@ -411,14 +406,13 @@ static const vshCmdOptDef opts_interface_name[] = {
 static bool
 cmdInterfaceName(vshControl *ctl, const vshCmd *cmd)
 {
-virInterfacePtr iface;
+g_autoptr(virshInterface) iface = NULL;
 
 if (!(iface = virshCommandOptInterfaceBy(ctl, cmd, NULL, NULL,
  VIRSH_BYMAC)))
 return false;
 
 vshPrint(ctl, "%s\n", virInterfaceGetName(iface));
-virInterfaceFree(iface);
 return true;
 }
 
@@ -448,14 +442,13 @@ static const vshCmdOptDef opts_interface_mac[] = {
 static bool
 cmdInterfaceMAC(vshControl *ctl, const vshCmd *cmd)
 {
-virInterfacePtr iface;
+g_autoptr(virshInterface) iface = NULL;
 
 if (!(iface = virshCommandOptInterfaceBy(ctl, cmd, NULL, NULL,
  VIRSH_BYNAME)))
 return false;
 
 vshPrint(ctl, "%s\n", virInterfaceGetMACString(iface));
-virInterfaceFree(iface);
 return true;
 }
 
@@ -484,7 +477,7 @@ static const vshCmdOptDef opts_interface_dumpxml[] = {
 static bool
 cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd)
 {
-virInterfacePtr iface;
+g_autoptr(virshInterface) iface = NULL;
 g_autofree char *dump = NULL;
 unsigned int flags = 0;
 
@@ -494,13 +487,10 @@ cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd)
 if (!(iface = virshCommandOptInterface(ctl, cmd, NULL)))
 return false;
 
-if (!(dump = virInterfaceGetXMLDesc(iface, flags))) {
-virInterfaceFree(iface);
+if (!(dump = virInterfaceGetXMLDesc(iface, flags)))
 return false;
-}
 
 vshPrint(ctl, "%s", dump);
-virInterfaceFree(iface);
 return true;
 }
 
@@ -530,7 +520,7 @@ static const vshCmdOptDef opts_interface_define[] = {
 static bool
 cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd)
 {
-virInterfacePtr iface;
+g_autoptr(virshInterface) iface = NULL;
 const char *from = NULL;
 g_autofree char *buffer = 

[PATCH 8/8] virsh: Add wrapper for virStreamFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk |  2 +-
 tools/virsh-console.c |  8 
 tools/virsh-domain.c  |  4 +---
 tools/virsh-util.c| 12 
 tools/virsh-util.h|  5 +
 tools/virsh-volume.c  |  8 ++--
 6 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 5daf5afcd0..cb12b64532 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|NodeDevice|NWFilter|Secret|StoragePool|StorageVol)Free\b'
 \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Network|NodeDevice|NWFilter|Secret|StoragePool|StorageVol|Stream)Free\b'
 \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 449619c95f..67ee608706 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -33,6 +33,7 @@
 # include "internal.h"
 # include "virsh.h"
 # include "virsh-console.h"
+# include "virsh-util.h"
 # include "virlog.h"
 # include "virfile.h"
 # include "viralloc.h"
@@ -117,8 +118,8 @@ virConsoleShutdown(virConsole *con,
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot terminate console stream"));
 }
-virStreamFree(con->st);
-con->st = NULL;
+
+g_clear_pointer(>st, virshStreamFree);
 }
 VIR_FREE(con->streamToTerminal.data);
 VIR_FREE(con->terminalToStream.data);
@@ -140,8 +141,7 @@ virConsoleDispose(void *obj)
 {
 virConsole *con = obj;
 
-if (con->st)
-virStreamFree(con->st);
+virshStreamFree(con->st);
 
 virCondDestroy(>cond);
 virResetError(>error);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index b1943b3875..0d4f7cc407 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5540,7 +5540,7 @@ cmdScreenshot(vshControl *ctl, const vshCmd *cmd)
 const char *name = NULL;
 char *file = NULL;
 int fd = -1;
-virStreamPtr st = NULL;
+g_autoptr(virshStream) st = NULL;
 unsigned int screen = 0;
 unsigned int flags = 0; /* currently unused */
 bool ret = false;
@@ -5610,8 +5610,6 @@ cmdScreenshot(vshControl *ctl, const vshCmd *cmd)
 unlink(file);
 if (generated)
 VIR_FREE(file);
-if (st)
-virStreamFree(st);
 VIR_FORCE_CLOSE(fd);
 VIR_FREE(mime);
 return ret;
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index fc2839d8fc..8fb617fa3c 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -362,6 +362,18 @@ virshStorageVolFree(virStorageVolPtr vol)
 }
 
 
+
+void
+virshStreamFree(virStreamPtr stream)
+{
+if (!stream)
+return;
+
+vshSaveLibvirtHelperError();
+virStreamFree(stream); /* sc_prohibit_obj_free_apis_in_virsh */
+}
+
+
 int
 virshDomainGetXMLFromDom(vshControl *ctl,
  virDomainPtr dom,
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 065055ddb1..838935d5e8 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -89,6 +89,11 @@ void
 virshStorageVolFree(virStorageVolPtr vol);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshStorageVol, virshStorageVolFree);
 
+typedef virStream virshStream;
+void
+virshStreamFree(virStreamPtr stream);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshStream, virshStreamFree);
+
 int
 virshDomainState(vshControl *ctl,
  virDomainPtr dom,
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index b896ebbbf9..70b6eac687 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -656,7 +656,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
 g_autoptr(virshStorageVol) vol = NULL;
 bool ret = false;
 int fd = -1;
-virStreamPtr st = NULL;
+g_autoptr(virshStream) st = NULL;
 const char *name = NULL;
 unsigned long long offset = 0, length = 0;
 virshControl *priv = ctl->privData;
@@ -731,8 +731,6 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
 ret = true;
 
  cleanup:
-if (st)
-virStreamFree(st);
 VIR_FORCE_CLOSE(fd);
 return ret;
 }
@@ -776,7 +774,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 g_autoptr(virshStorageVol) vol = NULL;
 bool ret = false;
 int fd = -1;
-virStreamPtr st = NULL;
+g_autoptr(virshStream) st = NULL;
 const char *name = NULL;
 unsigned long long offset = 0, length = 0;
 bool created = false;
@@ -851,8 +849,6 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 VIR_FORCE_CLOSE(fd);
 if (!ret && created)
 unlink(file);
-if (st)
-virStreamFree(st);
 return ret;

[PATCH 3/8] virsh: Add wrapper for virStoragePoolFree

[Michal Privoznik]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.

Signed-off-by: Michal Privoznik 
---
 build-aux/syntax-check.mk  |  2 +-
 tools/virsh-completer-pool.c   |  3 +-
 tools/virsh-completer-volume.c |  4 +-
 tools/virsh-domain.c   |  3 +-
 tools/virsh-pool.c | 67 +++---
 tools/virsh-util.c | 11 ++
 tools/virsh-util.h |  5 +++
 tools/virsh-volume.c   | 29 ---
 8 files changed, 51 insertions(+), 73 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 84cb895d86..111d2109e8 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -868,7 +868,7 @@ sc_gettext_init:
  $(_sc_search_regexp)
 
 sc_prohibit_obj_free_apis_in_virsh:
-   @prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret)Free\b' \
+   
@prohibit='\bvir(Domain|DomainSnapshot|Interface|Secret|StoragePool)Free\b' \
in_vc_files='virsh.*\.[ch]$$' \
exclude='sc_prohibit_obj_free_apis_in_virsh' \
halt='avoid using public virXXXFree in virsh, use virsh-prefixed 
wrappers instead' \
diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c
index 9350eff2d3..84e9d6cc5a 100644
--- a/tools/virsh-completer-pool.c
+++ b/tools/virsh-completer-pool.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-pool.h"
+#include "virsh-util.h"
 #include "conf/storage_conf.h"
 #include "viralloc.h"
 #include "virsh-pool.h"
@@ -61,7 +62,7 @@ virshStoragePoolNameCompleter(vshControl *ctl,
 ret = g_steal_pointer();
 
 for (i = 0; i < npools; i++)
-virStoragePoolFree(pools[i]);
+virshStoragePoolFree(pools[i]);
 g_free(pools);
 return ret;
 }
diff --git a/tools/virsh-completer-volume.c b/tools/virsh-completer-volume.c
index fcbc28b13b..1d83643c69 100644
--- a/tools/virsh-completer-volume.c
+++ b/tools/virsh-completer-volume.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "virsh-completer-volume.h"
+#include "virsh-util.h"
 #include "viralloc.h"
 #include "virsh-pool.h"
 #include "virsh.h"
@@ -32,7 +33,7 @@ virshStorageVolNameCompleter(vshControl *ctl,
  unsigned int flags)
 {
 virshControl *priv = ctl->privData;
-virStoragePoolPtr pool = NULL;
+g_autoptr(virshStoragePool) pool = NULL;
 virStorageVolPtr *vols = NULL;
 int rc;
 int nvols = 0;
@@ -63,7 +64,6 @@ virshStorageVolNameCompleter(vshControl *ctl,
 ret = g_steal_pointer();
 
  cleanup:
-virStoragePoolFree(pool);
 for (i = 0; i < nvols; i++)
 virStorageVolFree(vols[i]);
 g_free(vols);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0b78fbf728..461a5e19f6 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3801,7 +3801,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (pool) {
-virStoragePoolPtr storagepool = NULL;
+g_autoptr(virshStoragePool) storagepool = NULL;
 
 if (!source) {
 vshError(ctl,
@@ -3820,7 +3820,6 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 }
 
 vol.vol = virStorageVolLookupByName(storagepool, source);
-virStoragePoolFree(storagepool);
 
 } else {
vol.vol = virStorageVolLookupByPath(priv->conn, source);
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index fd9d5ead63..d391257f6e 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -20,6 +20,7 @@
 
 #include 
 #include "virsh-pool.h"
+#include "virsh-util.h"
 
 #include "internal.h"
 #include "virbuffer.h"
@@ -219,7 +220,7 @@ static const vshCmdOptDef opts_pool_autostart[] = {
 static bool
 cmdPoolAutostart(vshControl *ctl, const vshCmd *cmd)
 {
-virStoragePoolPtr pool;
+g_autoptr(virshStoragePool) pool = NULL;
 const char *name;
 int autostart;
 
@@ -233,7 +234,6 @@ cmdPoolAutostart(vshControl *ctl, const vshCmd *cmd)
 vshError(ctl, _("failed to mark pool %s as autostarted"), name);
 else
 vshError(ctl, _("failed to unmark pool %s as autostarted"), name);
-virStoragePoolFree(pool);
 return false;
 }
 
@@ -242,7 +242,6 @@ cmdPoolAutostart(vshControl *ctl, const vshCmd *cmd)
 else
 vshPrintExtra(ctl, _("Pool %s unmarked as autostarted\n"), name);
 
-virStoragePoolFree(pool);
 return true;
 }
 
@@ -271,7 +270,7 @@ static const vshCmdOptDef opts_pool_create[] = {
 static bool
 cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
 {
-virStoragePoolPtr pool;
+g_autoptr(virshStoragePool) pool = NULL;
 const char *from = NULL;
 g_autofree char *buffer = NULL;
 bool build;
@@ -307,7 +306,6 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
 
 vshPrintExtra(ctl, _("Pool %s created from %s\n"),
   virStoragePoolGetName(pool), from);
-virStoragePoolFree(pool);
 return 

[PATCH 0/8] virsh: Use g_autoptr() for public types

[Michal Privoznik]

In this patchset I'm switching from virXXXFree to g_autoptr(). There are
still some left, but very rare occurrence:

  libvirt.git $ git grep -o "vir[A-Z].*Free" -- tools/ | \
cut -d':' -f 2 | sort | uniq -c | sort -n

And of course, after these some functions could use subsequent cleanup,
e.g. because cleanup label collapsed to just return statement. But I
leave those for future work.

Michal Prívozník (8):
  virsh-util.h: Fix ordering of virshXXXFree functions
  virsh: Add wrapper for virInterfaceFree
  virsh: Add wrapper for virStoragePoolFree
  virsh: Add wrapper for virStorageVolFree
  virsh: Add wrapper for virNetworkFree
  virsh: Add wrapper for virNodeDeviceFree
  virsh: Add wrapper for virNWFilterFree
  virsh: Add wrapper for virStreamFree

 build-aux/syntax-check.mk |   4 +-
 tools/virsh-completer-interface.c |   3 +-
 tools/virsh-completer-network.c   |   8 +--
 tools/virsh-completer-nodedev.c   |   3 +-
 tools/virsh-completer-nwfilter.c  |   3 +-
 tools/virsh-completer-pool.c  |   3 +-
 tools/virsh-completer-volume.c|   8 +--
 tools/virsh-console.c |   8 +--
 tools/virsh-domain.c  |  10 +--
 tools/virsh-interface.c   |  55 +--
 tools/virsh-network.c |  75 ++--
 tools/virsh-nodedev.c |  49 --
 tools/virsh-nwfilter.c|  22 ++
 tools/virsh-pool.c|  67 ++
 tools/virsh-util.c|  78 +
 tools/virsh-util.h|  46 +++--
 tools/virsh-volume.c  | 109 --
 17 files changed, 254 insertions(+), 297 deletions(-)

-- 
2.32.0

[PATCH 1/8] virsh-util.h: Fix ordering of virshXXXFree functions

[Michal Privoznik]

Currently the order of virshXXXFree functions in the header file
does not correspond to the order in the corresponding .c file.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-util.h | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 6115b05d4d..f7e8116de3 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -40,16 +40,10 @@ virshCommandOptDomain(vshControl *ctl,
   const char **name);
 
 typedef virDomain virshDomain;
-
 void
 virshDomainFree(virDomainPtr dom);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshDomain, virshDomainFree);
 
-typedef virSecret virshSecret;
-void
-virshSecretFree(virSecretPtr secret);
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshSecret, virshSecretFree);
-
 typedef virDomainCheckpoint virshDomainCheckpoint;
 void
 virshDomainCheckpointFree(virDomainCheckpointPtr chk);
@@ -60,6 +54,11 @@ void
 virshDomainSnapshotFree(virDomainSnapshotPtr snap);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshDomainSnapshot, virshDomainSnapshotFree);
 
+typedef virSecret virshSecret;
+void
+virshSecretFree(virSecretPtr secret);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshSecret, virshSecretFree);
+
 int
 virshDomainState(vshControl *ctl,
  virDomainPtr dom,
-- 
2.32.0

Re: [PATCH v4 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller

[Ani Sinha]

On Sun, 26 Sep 2021, Laine Stump wrote:

> On 9/26/21 10:35 AM, Ani Sinha wrote:
> > This change introduces libvirt xml support to enable/disable hotplug on the
> > pci-root controller. It adds a 'target' subelement for the pci-root
> > controller
> > with a 'hotplug' property. This property can be used to enable or disable
> > hotplug for the pci-root controller. For example, in order to disable
> > hotplug
> > on the pci-root controller, one has to use set '' as
> > shown below:
> >
> > 
> >
> > 
> >
> > '' option would enable hotplug for pci-root controller.
> > This is also the default value. This option is only available for pc machine
> > types and is applicable for qemu only/kvm accelerator onlt.This feature was
> > introduced from qemu version 5.2 with the following change in qemu
> > repository:
> >
> > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on
> > the root bus")
> >
> > The above qemu commit describes some reasons why users might to disable
> > hotplug
> > on PCI root buses.
> >
> > Related unit tests to exercise the new conf option has also been added.
> >
> > Signed-off-by: Ani Sinha 
> > ---
> >   docs/formatdomain.rst | 12 
> >   docs/schemas/domaincommon.rng | 10 +++
> >   src/qemu/qemu_validate.c  |  9 +-
> >   .../pc-i440fx-acpi-root-hotplug-disable.xml   | 17 +++
> >   .../pc-i440fx-acpi-root-hotplug-enable.xml| 17 +++
> >   .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 +++
> >   tests/qemuxml2xmltest.c   |  2 ++
> >   7 files changed, 91 insertions(+), 6 deletions(-)
> >   create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
> >   create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
> >   create mode 100644
> > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 0f5d833521..e2a1768ba7 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU
> > only).`
> >  controller's "port" configuration value, which is visible to the
> > virtual
> >  machine. If set, port must be between 0 and 255.
> >   ``hotplug``
> > -   pcie-root-port and pcie-switch-downstream-port controllers can also have
> > a
> > -   ``hotplug`` attribute in the  subelement, which is used to
> > -   disable hotplug/unplug of devices on a particular controller. The
> > default
> > -   setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable
> > -   hotplug/unplug of devices on a particular controller. :since:`Since
> > 6.3.0`
> > +   pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`)
> > and
> > +   pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can
> > +   also have a ``hotplug`` attribute in the  subelement, which
> > is
> > +   used to disable hotplug/unplug of devices on a particular controller.
> > The
> > +   default setting of ``hotplug`` is ``on``; it should be set to ``off`` to
> > +   disable hotplug/unplug of devices on a particular controller.
> > +
> >   ``busNr``
> >  pci-expander-bus and pcie-expander-bus controllers can have an optional
> >  ``busNr`` attribute (1-254). This will be the bus number of the new
> > bus; All
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index fdc04f90aa..cce50c9c89 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -2668,6 +2668,16 @@
> >   
> > 
> >   
> > +
> > +
> > +  
> > +
> > +  
> > +
> > +  
> > +
> > +  
> > +
> > 
> > 
> >   
>
> This chunk  is unnecessary, since the schema already allows for a
> target/hotplug attribute for *all* PCI controller models (on line 2645), so
> this new bit in the scheme has no effect.
>
> (qemuValidateDomainDeviceDefControllerPCI is doing a more specific check to
> assure nobody tries to specify the hotplug attribute for controller models
> that don't support it, and you've added the proper check there)
>
>
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index 13fbfd01b2..510e766cfd 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > virDomainControllerDef *cont,
> >   /* hotplug */
> >   if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> >   switch ((virDomainControllerModelPCI) cont->model) {
> > +case 

Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller

[Ani Sinha]

On Sun, 26 Sep 2021, Laine Stump wrote:

> On 9/26/21 10:35 AM, Ani Sinha wrote:
> > This change adds qemu backend command line support for enabling or disabling
> > hotplug on the pci-root controller using the 'target' sub-element of the
> > pci-root controller as shown below:
> >
> > 
> >
> > 
> >
> > '' is only valid for pc (x86) machines and turns
> > on
> > the following command line option that is passed to qemu for x86 guests:
> >
> > -global PIIX4_PM.acpi-root-pci-hotplug=
> >
> > This change also adds the required qemuxml2argv unit tests in order to test
> > correct qemu arguments. Unit tests have also been added to test qemu
> > capability
> > validation checks.
> >
> > Signed-off-by: Ani Sinha 
> > ---
> >   src/qemu/qemu_command.c   | 12 +++
> >   .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
> >   .../pc-i440fx-acpi-root-hotplug-enable.err|  1 +
> >   tests/qemuxml2argvtest.c  |  6 
> >   4 files changed, 50 insertions(+)
> >   create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
> >   create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index b60ee1192b..0cdc10bf29 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -6029,6 +6029,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
> >  qemuDomainObjPrivate *priv)
> >   {
> >   virQEMUCaps *qemuCaps = priv->qemuCaps;
> > +int n;
> > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
> >   /* with new qemu we always want '-no-shutdown' on startup and we
> > set
> > @@ -6074,6 +6075,17 @@ qemuBuildPMCommandLine(virCommand *cmd,
> >  pm_object, def->pm.s4 ==
> > VIR_TRISTATE_BOOL_NO);
> >   }
> >   +for (n = 0; n < def->ncontrollers; n++) {
> > +virDomainControllerDef *cdef = def->controllers[n];
> > +if (cdef->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> > +cdef->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> > +cdef->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd,
> > "PIIX4_PM.acpi-root-pci-hotplug=%s",
> > +
> > virTristateSwitchTypeToString(cdef->opts.pciopts.hotplug));
> > +}
> > +}
> > +
>
> It would make more sense to include this in the commandline generator for PCI
> controllers, since that's where all other XML for PCI controllers is converted
> into a qemu commandline argument. That's a bit convoluted though,
> unfortunately.
>
> The loop going through all the controllers is in
> qemuBuildControllersByTypeCommandLine() which then calls
> qemuBuildControllerDevStr() and *that* is the function that builds the
> argument to the -device for each controller. For two reasons, we can't add the
> code for this new option alongside the other PCI controller commandline
> generation code in qemuBuildControllerDevStr() though:
>
> 1) the output of qemuBuildControllerDevStr() is assumed to be following a
> "-device" argument on the commandline, so we would end up with:
>
> "-device -global PIIX4_PM.acpi-root-pci-hotplug=off"
>
> which is *not* what we want.
>
> 2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls
> qemuBuildSkipController(), which tells the loop to skip generating any
> commandline for a pci-root controller (unless it's a P-series guest and the
> index is non-0).
>

Yes I saw this code and debated whether to put here or from the function
that adds PM commandlines. I put it there because I thought all PM related
options should go together. Also I did see qemuBuildSkipController() that
would have skipped through the pci-root controllers and I was not sure
philosophically if putting it qemuBuildControllersByTypeCommandLine()
would be right.

The loop yes, that was a bummer and I literally copied the loop over the
controllers from this code in qemuBuildControllersByTypeCommandLine().

> So the only way to make this work is to add a special case to
> qemuBuildControllersByTypeCommandLine() *before* the call to
> qemuBuildSkipController() - if the type is pci, model is pci-root, and index
> is 0 then conditionally add "-global", "PIIX4" and continue (skip the rest
> of the body of the loop).
>
> As with what you've already done here, this is also not 100% clean and
> consistent with the rest of the code, but since neither method is perfect I
> think putting it in the function that generates all the rest of the
> commandline args associated with PCI controllers is more logical and easier to
> follow.

OK now in v5 I have written it that way and I do realize that not having
to put another loop is a lot cleaner. Since you prefer this approach I
have made the changes in v5. I think its a tad bit better and simpler too.

[PATCH v5 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller

[Ani Sinha]

This change adds qemu backend command line support for enabling or disabling
hotplug on the pci-root controller using the 'target' sub-element of the
pci-root controller as shown below:


  


'' is only valid for pc (x86) machines and turns on
the following command line option that is passed to qemu for x86 guests:

-global PIIX4_PM.acpi-root-pci-hotplug=

This change also adds the required qemuxml2argv unit tests in order to test
correct qemu arguments. Unit tests have also been added to test qemu capability
validation checks.

Signed-off-by: Ani Sinha 
---
 src/qemu/qemu_command.c   | 17 ++
 .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
 .../pc-i440fx-acpi-root-hotplug-disable.err   |  1 +
 tests/qemuxml2argvtest.c  |  3 ++
 4 files changed, 52 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b60ee1192b..a8775bca24 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2645,6 +2645,20 @@ qemuBuildSkipController(const virDomainControllerDef 
*controller,
 return false;
 }
 
+static int
+qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd,
+  const virDomainControllerDef *controller)
+{
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
+controller->idx == 0 &&
+controller->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s",
+   
virTristateSwitchTypeToString(controller->opts.pciopts.hotplug));
+}
+return 0;
+}
 
 static int
 qemuBuildControllersByTypeCommandLine(virCommand *cmd,
@@ -2661,6 +2675,9 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd,
 if (cont->type != type)
 continue;
 
+if (qemuBuildPMPCIRootHotplugCommandLine(cmd, cont))
+continue;
+
 if (qemuBuildSkipController(cont, def))
 continue;
 
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
new file mode 100644
index 00..dd8ea503fc
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
+-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
+-m 1024 \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-global PIIX4_PM.acpi-root-pci-hotplug=off \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
new file mode 100644
index 00..827c93a7ba
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
@@ -0,0 +1 @@
+unsupported configuration: setting the hotplug property on a pci-root device 
is not supported by this QEMU binary
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 13e387df3f..d5ddd60182 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2571,6 +2571,9 @@ mymain(void)
 QEMU_CAPS_DEVICE_IOH3420,
 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI,
 QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4);
+DO_TEST("pc-i440fx-acpi-root-hotplug-disable",
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-disable");
 DO_TEST("q35-usb2",
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
-- 
2.25.1

[PATCH v5 4/4] NEWS: release note the new hotplug enable/disable option on pci-root controller

[Ani Sinha]

A new 'target' subelement of the pci-root controller has been introduced having
a 'hotplug' property. This proprty can be used to turn off or turn on hotplug
capability of the devices plugged into the pci-root ports. This change release
notes this feature for the next release.

The new element can be used like this:


   


This will turn off hotplug capability on the pci-root ports. To turn the
capability on, we set hotplug='on' above (which is also the default).

Signed-off-by: Ani Sinha 
---
 NEWS.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index fd20e50d18..b244cbb2be 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -29,6 +29,12 @@ v7.8.0 (unreleased)
 active. This information can also be retrieved with the new virsh command
 ``nodedev-info``.
 
+  * qemu: support disabling hotplug of devices on the pci-root controller
+
+libvirt can now set the "hotplug" option for pci-root controller which can
+be used to disable hotplug/unplug of devices from the pci root port. The
+default behavior is to keep hotplug on pci-root ports enabled.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.25.1

[PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller

[Ani Sinha]

This change introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:


  


'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses.

Related unit tests to exercise the new conf option has also been added.

Signed-off-by: Ani Sinha 
---
 docs/formatdomain.rst | 12 
 src/qemu/qemu_validate.c  |  9 +-
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 +++
 .../pc-i440fx-acpi-root-hotplug-enable.xml| 30 +++
 .../pc-i440fx-acpi-root-hotplug-disable.xml   |  1 +
 .../pc-i440fx-acpi-root-hotplug-enable.xml|  1 +
 tests/qemuxml2xmltest.c   |  4 +++
 7 files changed, 81 insertions(+), 6 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 0f5d833521..e2a1768ba7 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).`
controller's "port" configuration value, which is visible to the virtual
machine. If set, port must be between 0 and 255.
 ``hotplug``
-   pcie-root-port and pcie-switch-downstream-port controllers can also have a
-   ``hotplug`` attribute in the  subelement, which is used to
-   disable hotplug/unplug of devices on a particular controller. The default
-   setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable
-   hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0`
+   pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and
+   pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can
+   also have a ``hotplug`` attribute in the  subelement, which is
+   used to disable hotplug/unplug of devices on a particular controller. The
+   default setting of ``hotplug`` is ``on``; it should be set to ``off`` to
+   disable hotplug/unplug of devices on a particular controller.
+
 ``busNr``
pci-expander-bus and pcie-expander-bus controllers can have an optional
``busNr`` attribute (1-254). This will be the bus number of the new bus; All
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 13fbfd01b2..510e766cfd 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const 
virDomainControllerDef *cont,
 /* hotplug */
 if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
 switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("setting the %s property on a pci-root device 
is not supported by this QEMU binary"),
+   "hotplug");
+return -1;
+}
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
@@ -3889,7 +3897,6 @@ qemuValidateDomainDeviceDefControllerPCI(const 
virDomainControllerDef *cont,
 }
 break;
 
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
 case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
new file mode 100644
index 00..93f2779f68
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
@@ -0,0 +1,30 @@
+
+  i440fx
+  56f5055c-1b8d-490c-844a-ad646a1c
+  1048576
+  1048576
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+

[PATCH v5 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines

[Ani Sinha]

The following change in qemu added support for a global boolean flag specific
to i440fx machines that would turn off or on acpi based hotplug for pci root
bus:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu
commandline. It is enabled by default. This patch adds the corresponding qemu
capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.

Please note that the test specific qemu capabilities .replies files has already
been updated as a part of regular refreshing them when a new qemu version is
released. Hence, no updates to those files are required.

Signed-off-by: Ani Sinha 
Reviewed-by: Laine Stump 
---
 src/qemu/qemu_capabilities.c | 4 
 src/qemu/qemu_capabilities.h | 3 +++
 tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
 5 files changed, 10 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index db5432c9fc..71aca20c4c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
   "set-action", /* QEMU_CAPS_SET_ACTION */
   "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
+
+  /* 410 */
+  "piix4-acpi-root-hotplug-en", /* 
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
 );
 
 
@@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags 
virQEMUCapsDevicePropsIDEDrive[] = {
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = {
 { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
 { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
+{ "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL },
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 097f28bd40..c2d1e352bd 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
 QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
 
+/* 410 */
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc 
PIIX4_PM.acpi-root-pci-hotplug */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index e09880e937..ffd0e66d00 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -233,6 +233,7 @@
   
   
   
+  
   5002000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index 571336c1fa..658a1e742f 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -241,6 +241,7 @@
   
   
   
+  
   600
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 74b87847d0..5bb21fec47 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -243,6 +243,7 @@
   
   
   
+  
   6001000
   0
   43100243
-- 
2.25.1

[PATCH v5 0/4] Add support to enable/disable hotplug on pci-root controller

[Ani Sinha]

changelog:

v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command 
line
is now added in a new function and called from 
qemuBuildControllersByTypeCommandLine().
Output xmls are now symlinked to input xmls for unit tests.
v4: split the original patchset into a pci-root controller specific patch 
series.
also the libvirt conf is now a sub-element of the pci-root controller as was
suggested by Dan Berrange. Please see discussion here:
https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html
v3: reorganized the patches as per Laine's suggestion. Added more
details in commit messages. Added conf description in formatdomain.rst.
Added changelog for next release.
v2: fixed bugs and added additional missing unit tests.
v1: initial implementation. Had some bugs and missed some unit tests

This patchset introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:


  


'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses [1].

The corresponding commandline option to qemu for x86 guests is:

-global PIIX4_PM.acpi-root-pci-hotplug=

Notes:

1. The use case scenario described by Laine in
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html
intentionally does not discuss i440fx and focusses solely on q35. I do realize
that redhat has moved on from i440fx and currently efforts for new features
are concentrated on q35 machines only. We have had some hard debates on this
on the qemu mailing list before. The fact of the matter is that i440fx is
not at 1-1 parity with q35. There are many users who are currenly using i440fx
and are simply not ready to move to q35 without sacrificing some
existing features they support today. For example
https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations.
https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more
information on the differences. Hence we need to solve the issue Laine has
described in the above email for i440fx without adding additional bridges.

Further, in  Daniel Berrange's words from :
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html

"From the upstream POV, there's been no decision / agreement to phase
out PIIX, this is purely a RHEL downstream decision & plan. If other
distros / users have a different POV, and find the feature useful, we
should accept the patch if it meets the normal QEMU patch requirements.
"

Also to be noted that I have already experimented this qemu commandline option
using libvirt passthrough feature as has been documented in
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
This was only meant to be a short term solution until libvirt started
supporting this natively. Supporting this option through libvirt would simplify
their use case as well as add capability validations
and graceful failure scenarios in case qemu did not support the option.


Ani Sinha (4):
  qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx
machines
  conf: introduce option to enable/disable pci hotplug on pci-root
controller
  qemu: command: add support to enable/disable hotplug on pci-root
controller
  NEWS: release note the new hotplug enable/disable option on pci-root
controller

 NEWS.rst  |  6 
 docs/formatdomain.rst | 12 ---
 src/qemu/qemu_capabilities.c  |  4 +++
 src/qemu/qemu_capabilities.h  |  3 ++
 src/qemu/qemu_command.c   | 17 ++
 src/qemu/qemu_validate.c  |  9 +-
 .../caps_5.2.0.x86_64.xml |  1 +
 .../caps_6.0.0.x86_64.xml |  1 +
 .../caps_6.1.0.x86_64.xml |  1 +
 .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
 .../pc-i440fx-acpi-root-hotplug-disable.err   |  1 +
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 ++
 .../pc-i440fx-acpi-root-hotplug-enable.xml| 30 ++
 tests/qemuxml2argvtest.c  |  3 ++
 .../pc-i440fx-acpi-root-hotplug-disable.xml   |  1 +
 .../pc-i440fx-acpi-root-hotplug-enable.xml|  1 +
 tests/qemuxml2xmltest.c   |  4 

Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller

[Laine Stump]

On 9/26/21 10:35 AM, Ani Sinha wrote:

This change adds qemu backend command line support for enabling or disabling
hotplug on the pci-root controller using the 'target' sub-element of the
pci-root controller as shown below:


   


'' is only valid for pc (x86) machines and turns on
the following command line option that is passed to qemu for x86 guests:

-global PIIX4_PM.acpi-root-pci-hotplug=

This change also adds the required qemuxml2argv unit tests in order to test
correct qemu arguments. Unit tests have also been added to test qemu capability
validation checks.

Signed-off-by: Ani Sinha 
---
  src/qemu/qemu_command.c   | 12 +++
  .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
  .../pc-i440fx-acpi-root-hotplug-enable.err|  1 +
  tests/qemuxml2argvtest.c  |  6 
  4 files changed, 50 insertions(+)
  create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
  create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b60ee1192b..0cdc10bf29 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6029,6 +6029,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
 qemuDomainObjPrivate *priv)
  {
  virQEMUCaps *qemuCaps = priv->qemuCaps;
+int n;
  
  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {

  /* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6074,6 +6075,17 @@ qemuBuildPMCommandLine(virCommand *cmd,
 pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
  }
  
+for (n = 0; n < def->ncontrollers; n++) {

+virDomainControllerDef *cdef = def->controllers[n];
+if (cdef->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+cdef->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
+cdef->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s",
+   
virTristateSwitchTypeToString(cdef->opts.pciopts.hotplug));
+}
+}
+


It would make more sense to include this in the commandline generator 
for PCI controllers, since that's where all other XML for PCI 
controllers is converted into a qemu commandline argument. That's a bit 
convoluted though, unfortunately.


The loop going through all the controllers is in 
qemuBuildControllersByTypeCommandLine() which then calls 
qemuBuildControllerDevStr() and *that* is the function that builds the 
argument to the -device for each controller. For two reasons, we can't 
add the code for this new option alongside the other PCI controller 
commandline generation code in qemuBuildControllerDevStr() though:


1) the output of qemuBuildControllerDevStr() is assumed to be following 
a "-device" argument on the commandline, so we would end up with:


"-device -global PIIX4_PM.acpi-root-pci-hotplug=off"

which is *not* what we want.

2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls 
qemuBuildSkipController(), which tells the loop to skip generating any 
commandline for a pci-root controller (unless it's a P-series guest and 
the index is non-0).


So the only way to make this work is to add a special case to 
qemuBuildControllersByTypeCommandLine() *before* the call to 
qemuBuildSkipController() - if the type is pci, model is pci-root, and 
index is 0 then conditionally add "-global", "PIIX4" and continue 
(skip the rest of the body of the loop).


As with what you've already done here, this is also not 100% clean and 
consistent with the rest of the code, but since neither method is 
perfect I think putting it in the function that generates all the rest 
of the commandline args associated with PCI controllers is more logical 
and easier to follow. (If anyone disagrees and thinks that the 
commandline for this should be generated in the place this patch already 
does it, please speak up!)




  return 0;
  }
  
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args

new file mode 100644
index 00..1fb68381d9
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
+-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
+-m 1024 \
+-realtime mlock=off \
+-smp 

Re: [PATCH v4 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller

[Laine Stump]

On 9/26/21 10:35 AM, Ani Sinha wrote:

This change introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:


   


'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root 
bus")

The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses.

Related unit tests to exercise the new conf option has also been added.

Signed-off-by: Ani Sinha 
---
  docs/formatdomain.rst | 12 
  docs/schemas/domaincommon.rng | 10 +++
  src/qemu/qemu_validate.c  |  9 +-
  .../pc-i440fx-acpi-root-hotplug-disable.xml   | 17 +++
  .../pc-i440fx-acpi-root-hotplug-enable.xml| 17 +++
  .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 +++
  tests/qemuxml2xmltest.c   |  2 ++
  7 files changed, 91 insertions(+), 6 deletions(-)
  create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
  create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
  create mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 0f5d833521..e2a1768ba7 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).`
 controller's "port" configuration value, which is visible to the virtual
 machine. If set, port must be between 0 and 255.
  ``hotplug``
-   pcie-root-port and pcie-switch-downstream-port controllers can also have a
-   ``hotplug`` attribute in the  subelement, which is used to
-   disable hotplug/unplug of devices on a particular controller. The default
-   setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable
-   hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0`
+   pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and
+   pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can
+   also have a ``hotplug`` attribute in the  subelement, which is
+   used to disable hotplug/unplug of devices on a particular controller. The
+   default setting of ``hotplug`` is ``on``; it should be set to ``off`` to
+   disable hotplug/unplug of devices on a particular controller.
+
  ``busNr``
 pci-expander-bus and pcie-expander-bus controllers can have an optional
 ``busNr`` attribute (1-254). This will be the bus number of the new bus; 
All
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index fdc04f90aa..cce50c9c89 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2668,6 +2668,16 @@
  

  
+
+
+  
+
+  
+
+  
+
+  
+


  


This chunk  is unnecessary, since the schema already allows for a 
target/hotplug attribute for *all* PCI controller models (on line 2645), 
so this new bit in the scheme has no effect.


(qemuValidateDomainDeviceDefControllerPCI is doing a more specific check 
to assure nobody tries to specify the hotplug attribute for controller 
models that don't support it, and you've added the proper check there)




diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 13fbfd01b2..510e766cfd 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const 
virDomainControllerDef *cont,
  /* hotplug */
  if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
  switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("setting the %s property on a pci-root device is 
not supported by this QEMU binary"),
+   "hotplug");
+return -1;
+}
+break;
  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
  case 

[PATCH v4 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines

[Ani Sinha]

The following change in qemu added support for a global boolean flag specific
to i440fx machines that would turn off or on acpi based hotplug for pci root
bus:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu
commandline. It is enabled by default. This patch adds the corresponding qemu
capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.

Please note that the test specific qemu capabilities .replies files has already
been updated as a part of regular refreshing them when a new qemu version is
released. Hence, no updates to those files are required.

Signed-off-by: Ani Sinha 
Reviewed-by: Laine Stump 
---
 src/qemu/qemu_capabilities.c | 4 
 src/qemu/qemu_capabilities.h | 3 +++
 tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
 5 files changed, 10 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index db5432c9fc..71aca20c4c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
   "set-action", /* QEMU_CAPS_SET_ACTION */
   "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
+
+  /* 410 */
+  "piix4-acpi-root-hotplug-en", /* 
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
 );
 
 
@@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags 
virQEMUCapsDevicePropsIDEDrive[] = {
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = {
 { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
 { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
+{ "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL },
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 097f28bd40..c2d1e352bd 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
 QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
 
+/* 410 */
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc 
PIIX4_PM.acpi-root-pci-hotplug */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index e09880e937..ffd0e66d00 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -233,6 +233,7 @@
   
   
   
+  
   5002000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index 571336c1fa..658a1e742f 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -241,6 +241,7 @@
   
   
   
+  
   600
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 74b87847d0..5bb21fec47 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -243,6 +243,7 @@
   
   
   
+  
   6001000
   0
   43100243
-- 
2.25.1

[PATCH v4 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller

[Ani Sinha]

This change introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:


  


'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses.

Related unit tests to exercise the new conf option has also been added.

Signed-off-by: Ani Sinha 
---
 docs/formatdomain.rst | 12 
 docs/schemas/domaincommon.rng | 10 +++
 src/qemu/qemu_validate.c  |  9 +-
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 17 +++
 .../pc-i440fx-acpi-root-hotplug-enable.xml| 17 +++
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 +++
 tests/qemuxml2xmltest.c   |  2 ++
 7 files changed, 91 insertions(+), 6 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
 create mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 0f5d833521..e2a1768ba7 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).`
controller's "port" configuration value, which is visible to the virtual
machine. If set, port must be between 0 and 255.
 ``hotplug``
-   pcie-root-port and pcie-switch-downstream-port controllers can also have a
-   ``hotplug`` attribute in the  subelement, which is used to
-   disable hotplug/unplug of devices on a particular controller. The default
-   setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable
-   hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0`
+   pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and
+   pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can
+   also have a ``hotplug`` attribute in the  subelement, which is
+   used to disable hotplug/unplug of devices on a particular controller. The
+   default setting of ``hotplug`` is ``on``; it should be set to ``off`` to
+   disable hotplug/unplug of devices on a particular controller.
+
 ``busNr``
pci-expander-bus and pcie-expander-bus controllers can have an optional
``busNr`` attribute (1-254). This will be the bus number of the new bus; All
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index fdc04f90aa..cce50c9c89 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2668,6 +2668,16 @@
 
   
 
+
+
+  
+
+  
+
+  
+
+  
+
   
   
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 13fbfd01b2..510e766cfd 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const 
virDomainControllerDef *cont,
 /* hotplug */
 if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
 switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("setting the %s property on a pci-root device 
is not supported by this QEMU binary"),
+   "hotplug");
+return -1;
+}
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
@@ -3889,7 +3897,6 @@ qemuValidateDomainDeviceDefControllerPCI(const 
virDomainControllerDef *cont,
 }
 break;
 
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
 case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 case 

[PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller

[Ani Sinha]

This change adds qemu backend command line support for enabling or disabling
hotplug on the pci-root controller using the 'target' sub-element of the
pci-root controller as shown below:


  


'' is only valid for pc (x86) machines and turns on
the following command line option that is passed to qemu for x86 guests:

-global PIIX4_PM.acpi-root-pci-hotplug=

This change also adds the required qemuxml2argv unit tests in order to test
correct qemu arguments. Unit tests have also been added to test qemu capability
validation checks.

Signed-off-by: Ani Sinha 
---
 src/qemu/qemu_command.c   | 12 +++
 .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
 .../pc-i440fx-acpi-root-hotplug-enable.err|  1 +
 tests/qemuxml2argvtest.c  |  6 
 4 files changed, 50 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b60ee1192b..0cdc10bf29 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6029,6 +6029,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
 {
 virQEMUCaps *qemuCaps = priv->qemuCaps;
+int n;
 
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
 /* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6074,6 +6075,17 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
 }
 
+for (n = 0; n < def->ncontrollers; n++) {
+virDomainControllerDef *cdef = def->controllers[n];
+if (cdef->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+cdef->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
+cdef->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s",
+   
virTristateSwitchTypeToString(cdef->opts.pciopts.hotplug));
+}
+}
+
 return 0;
 }
 
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
new file mode 100644
index 00..1fb68381d9
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
+-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
+-m 1024 \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-root-pci-hotplug=off \
+-boot strict=on \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
new file mode 100644
index 00..827c93a7ba
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err
@@ -0,0 +1 @@
+unsupported configuration: setting the hotplug property on a pci-root device 
is not supported by this QEMU binary
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 13e387df3f..e1b07f591f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2571,6 +2571,12 @@ mymain(void)
 QEMU_CAPS_DEVICE_IOH3420,
 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI,
 QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4);
+DO_TEST("pc-i440fx-acpi-root-hotplug-disable",
+QEMU_CAPS_DEVICE_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_IOH3420,
+QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable");
 DO_TEST("q35-usb2",
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
-- 
2.25.1

[PATCH v4 4/4] NEWS: release note the new hotplug enable/disable option on pci-root controller

[Ani Sinha]

A new 'target' subelement of the pci-root controller has been introduced having
a 'hotplug' property. This proprty can be used to turn off or turn on hotplug
capability of the devices plugged into the pci-root ports. This change release
notes this feature for the next release.

The new element can be used like this:


   


This will turn off hotplug capability on the pci-root ports. To turn the
capability on, we set hotplug='on' above (which is also the default).

Signed-off-by: Ani Sinha 
---
 NEWS.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index fd20e50d18..b244cbb2be 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -29,6 +29,12 @@ v7.8.0 (unreleased)
 active. This information can also be retrieved with the new virsh command
 ``nodedev-info``.
 
+  * qemu: support disabling hotplug of devices on the pci-root controller
+
+libvirt can now set the "hotplug" option for pci-root controller which can
+be used to disable hotplug/unplug of devices from the pci root port. The
+default behavior is to keep hotplug on pci-root ports enabled.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.25.1

[PATCH v4 0/4] Add support to enable/disable hotplug on pci-root controller

[Ani Sinha]

changelog:

v4: split the original patchset into a pci-root controller specific patch 
series.
also the libvirt conf is now a sub-element of the pci-root controller as was
suggested by Dan Berrange. Please see discussion here:
https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html
v3: reorganized the patches as per Laine's suggestion. Added more
details in commit messages. Added conf description in formatdomain.rst.
Added changelog for next release.
v2: fixed bugs and added additional missing unit tests.
v1: initial implementation. Had some bugs and missed some unit tests

This patchset introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:


  


'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:

3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the 
root bus")

The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses [1].

The corresponding commandline option to qemu for x86 guests is:

-global PIIX4_PM.acpi-root-pci-hotplug=

Notes:

1. The use case scenario described by Laine in
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html
intentionally does not discuss i440fx and focusses solely on q35. I do realize
that redhat has moved on from i440fx and currently efforts for new features
are concentrated on q35 machines only. We have had some hard debates on this
on the qemu mailing list before. The fact of the matter is that i440fx is
not at 1-1 parity with q35. There are many users who are currenly using i440fx
and are simply not ready to move to q35 without sacrificing some
existing features they support today. For example
https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations.
https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more
information on the differences. Hence we need to solve the issue Laine has
described in the above email for i440fx without adding additional bridges.

Further, in  Daniel Berrange's words from :
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html

"From the upstream POV, there's been no decision / agreement to phase
out PIIX, this is purely a RHEL downstream decision & plan. If other
distros / users have a different POV, and find the feature useful, we
should accept the patch if it meets the normal QEMU patch requirements.
"

Also to be noted that I have already experimented this qemu commandline option
using libvirt passthrough feature as has been documented in
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
This was only meant to be a short term solution until libvirt started
supporting this natively. Supporting this option through libvirt would simplify
their use case as well as add capability validations
and graceful failure scenarios in case qemu did not support the option.

Ani Sinha (4):
  qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx
machines
  conf: introduce option to enable/disable pci hotplug on pci-root
controller
  qemu: command: add support to enable/disable hotplug on pci-root
controller
  NEWS: release note the new hotplug enable/disable option on pci-root
controller

 NEWS.rst  |  6 
 docs/formatdomain.rst | 12 ---
 docs/schemas/domaincommon.rng | 10 ++
 src/qemu/qemu_capabilities.c  |  4 +++
 src/qemu/qemu_capabilities.h  |  3 ++
 src/qemu/qemu_command.c   | 12 +++
 src/qemu/qemu_validate.c  |  9 +-
 .../caps_5.2.0.x86_64.xml |  1 +
 .../caps_6.0.0.x86_64.xml |  1 +
 .../caps_6.1.0.x86_64.xml |  1 +
 .../pc-i440fx-acpi-root-hotplug-disable.args  | 31 +++
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 17 ++
 .../pc-i440fx-acpi-root-hotplug-enable.err|  1 +
 .../pc-i440fx-acpi-root-hotplug-enable.xml| 17 ++
 tests/qemuxml2argvtest.c  |  6 
 .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 ++
 tests/qemuxml2xmltest.c   |  2 ++
 17 files changed, 157 insertions(+), 6 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
 create mode 

[PATCH] qemu: ingore the transient domain state in fake reboot

[Zhenzhong Duan]

When action for 'on_poweroff' is set to 'restart', 'fake reboot'
is triggered and qemu shutdown state is transient. Domain state
need not to be changed and events not sent in this case.

Fixes:4ffc807214cb80086d57e1d3e7b60959a41d2874
Signed-off-by: Zhenzhong Duan 
---
 src/qemu/qemu_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 632cb817b9..fcdda4ffe1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -612,7 +612,7 @@ qemuProcessHandleShutdown(qemuMonitor *mon G_GNUC_UNUSED,
 
 /* In case of fake reboot qemu shutdown state is transient so don't
  * change domain state nor send events. */
-if (!priv->fakeReboot ||
+if (!priv->fakeReboot &&
 vm->def->onPoweroff != VIR_DOMAIN_LIFECYCLE_ACTION_RESTART) {
 VIR_DEBUG("Transitioned guest %s to shutdown state",
   vm->def->name);
-- 
2.25.1