[libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Tobin Feldman-Fitzthum]

Only probe QEMU binary with accel=tcg if TCG is not disabled.
Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG
is available.

Signed-off-by: Tobin Feldman-Fitzthum 
---
 src/qemu/qemu_capabilities.c | 22 ++
 src/qemu/qemu_capabilities.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index fe311048d4..c56b2d8f0e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "fsdev.multidevs",
   "virtio.packed",
   "pcie-root-port.hotplug",
+  "tcg-disabled",
 );
 
 
@@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
 virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,
  true, false);
 
-if (virCapabilitiesAddGuestDomain(guest,
-  VIR_DOMAIN_VIRT_QEMU,
-  NULL,
-  NULL,
-  0,
-  NULL) == NULL)
-goto cleanup;
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) {
+if (virCapabilitiesAddGuestDomain(guest,
+  VIR_DOMAIN_VIRT_QEMU,
+  NULL,
+  NULL,
+  0,
+  NULL) == NULL) {
+goto cleanup;
+}
+}
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 if (virCapabilitiesAddGuestDomain(guest,
@@ -2295,7 +2299,8 @@ bool
 virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
virDomainVirtType virtType)
 {
-if (virtType == VIR_DOMAIN_VIRT_QEMU)
+if (virtType == VIR_DOMAIN_VIRT_QEMU &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
 return true;
 
 if (virtType == VIR_DOMAIN_VIRT_KVM &&
@@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
  * off.
  */
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) &&
 virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0)
 return -1;
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 1681fc79a8..abc4ee82cb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */
 QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
 QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */
+QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
2.20.1 (Apple Git-117)

[libvirt PATCH v4 0/3] QEMU: support QEMU built without TCG

[Tobin Feldman-Fitzthum]

Rebased and resent from about a month ago.

Since version 2.10, QEMU can be built without TCG. This patch
adds capabillity QEMU_CAPS_TCG_DISABLED that allows libvirt
to use a QEMU built without TCG.

Rather than create a capability that is set whenever TCG is
enabled (almost always), QEMU_CAPS_TCG_DISABLED is set only
when the TCG is not available. This avoids some issues
with backwards compatability.

For a domain that was created using QEMU >= 2.10 with KVM,
there is no information in the cached XML file that says
whether or not TCG was enabled. Versions of QEMU older
than 2.10 do not have a QMP interface for determining
whether QEMU is available. Since QEMU_CAPS_TCG_DISABLED
is set only when TCG is disabled, we do not have to do any
extra work to infer an appropriate value in either of these
cases.

New in vesion 4:
Move virQEMUCapsProbeQMPTCGState into
virQEMUCapsProbeQMPDevices so that we can reuse the qom call.
Rename, virQEMUCapsProbeQMPDevices to virQEMUProbeQMPTypes.
All patches compile.

Tobin Feldman-Fitzthum (3):
  add QEMU_CAPS_TCG_DISABLED and probe conditionally
  add virQEMUCapsGetVirtType convenience function
  add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED

 src/qemu/qemu_capabilities.c | 81 +++-
 src/qemu/qemu_capabilities.h |  3 ++
 2 files changed, 65 insertions(+), 19 deletions(-)

-- 
2.20.1 (Apple Git-117)

[libvirt PATCH v4 3/3] add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED

[Tobin Feldman-Fitzthum]

Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version 
is > 2.10, KVM is enabled, and tcg-accel is not present in 
qom-list-types result.

Signed-off-by: Tobin Feldman-Fitzthum 
---
 src/qemu/qemu_capabilities.c | 34 --
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e7179ea048..4a3170fc5c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps,
 }
 
 static int
-virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
+virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
+char **values,
+int nvalues)
+{
+size_t i;
+bool found = false;
+/*
+ * As of version 2.10, QEMU can be built without the TCG.
+ * */
+if (qemuCaps->version < 201)
+return 0;
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+return 0;
+
+for (i = 0; i < nvalues; i++) {
+if (STREQ(values[i], "tcg-accel")) {
+found = true;
+break;
+}
+}
+
+if (!found)
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED);
+
+return 0;
+}
+
+static int
+virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
 {
 int nvalues;
@@ -2584,6 +2612,8 @@ virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
 
 if ((nvalues = qemuMonitorGetObjectTypes(mon, &values)) < 0)
 return -1;
+
+virQEMUCapsProbeQMPTCGState(qemuCaps, values, nvalues);
 virQEMUCapsProcessStringFlags(qemuCaps,
   G_N_ELEMENTS(virQEMUCapsObjectTypes),
   virQEMUCapsObjectTypes,
@@ -5047,7 +5077,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
 
 if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
 return -1;
-if (virQEMUCapsProbeQMPDevices(qemuCaps, mon) < 0)
+if (virQEMUCapsProbeQMPTypes(qemuCaps, mon) < 0)
 return -1;
 if (virQEMUCapsProbeQMPMachineTypes(qemuCaps, type, mon) < 0)
 return -1;
-- 
2.20.1 (Apple Git-117)

[libvirt PATCH v4 2/3] add virQEMUCapsGetVirtType convenience function

[Tobin Feldman-Fitzthum]

Signed-off-by: Tobin Feldman-Fitzthum 
---
 src/qemu/qemu_capabilities.c | 25 -
 src/qemu/qemu_capabilities.h |  2 ++
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c56b2d8f0e..e7179ea048 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4984,6 +4984,20 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCapsPtr 
qemuCaps,
 #define QEMU_MIN_MINOR 5
 #define QEMU_MIN_MICRO 0
 
+virDomainVirtType
+virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps)
+{
+virDomainVirtType type;
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+type = VIR_DOMAIN_VIRT_KVM;
+else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
+type = VIR_DOMAIN_VIRT_QEMU;
+else
+type = VIR_DOMAIN_VIRT_NONE;
+
+return type;
+}
+
 int
 virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
   qemuMonitorPtr mon)
@@ -5028,11 +5042,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
 if (virQEMUCapsProbeQMPKVMState(qemuCaps, mon) < 0)
 return -1;
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
-type = VIR_DOMAIN_VIRT_KVM;
-else
-type = VIR_DOMAIN_VIRT_QEMU;
-
+type = virQEMUCapsGetVirtType(qemuCaps);
 accel = virQEMUCapsGetAccel(qemuCaps, type);
 
 if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
@@ -5525,10 +5535,7 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
 goto cleanup;
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
-capsType = VIR_DOMAIN_VIRT_KVM;
-else
-capsType = VIR_DOMAIN_VIRT_QEMU;
+capsType = virQEMUCapsGetVirtType(qemuCaps);
 
 if (virttype == VIR_DOMAIN_VIRT_NONE)
 virttype = capsType;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index abc4ee82cb..9a779912fe 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -636,6 +636,8 @@ int virQEMUCapsGetCPUFeatures(virQEMUCapsPtr qemuCaps,
   bool migratable,
   char ***features);
 
+virDomainVirtType virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps);
+
 bool virQEMUCapsIsArchSupported(virQEMUCapsPtr qemuCaps,
 virArch arch);
 bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
-- 
2.20.1 (Apple Git-117)

[PATCH v3 1/2] conf: Add option for settings

[Julio Faracco]

If an user is trying to configure a dhcp neetwork settings, it is not
possible to change the leasetime of a range or a host entry. This is
available using dnsmasq extra options, but they are associated with
dhcp-range or dhcp-hosts fields. This patch implements a leasetime for
range and hosts tags. They can be defined under that settings:


  

  
  

  


Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446

Signed-off-by: Julio Faracco 
---
 docs/schemas/basictypes.rng |   8 ++
 docs/schemas/network.rng|  20 +
 src/conf/network_conf.c | 159 +++-
 src/conf/network_conf.h |  27 +-
 src/libvirt_private.syms|   3 +
 src/network/bridge_driver.c |  56 +++--
 src/network/bridge_driver.h |   1 +
 src/test/test_driver.c  |   2 +-
 src/util/virdnsmasq.c   |  60 +-
 src/util/virdnsmasq.h   |   3 +
 src/vbox/vbox_network.c |  16 ++--
 tests/networkxml2conftest.c |  15 ++--
 12 files changed, 306 insertions(+), 64 deletions(-)

diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 81465273c8..271ed18afb 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -607,4 +607,12 @@
 
   
 
+  
+
+  seconds
+  mins
+  hours
+
+  
+
 
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 60453225d6..88b6f4dfdd 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -371,6 +371,16 @@
   
 
 
+
+  
+
+  
+  
+
+  
+
+  
+
   
 
 
@@ -388,6 +398,16 @@
   
 
 
+
+  
+
+  
+  
+
+  
+
+  
+
   
 
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 819b645df7..286a0edb7c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -70,6 +70,13 @@ VIR_ENUM_IMPL(virNetworkTaint,
   "hook-script",
 );
 
+VIR_ENUM_IMPL(virNetworkDHCPLeaseTimeUnit,
+  VIR_NETWORK_DHCP_LEASETIME_UNIT_LAST,
+  "seconds",
+  "mins",
+  "hours",
+);
+
 static virClassPtr virNetworkXMLOptionClass;
 
 static void
@@ -132,12 +139,20 @@ virNetworkForwardPfDefClear(virNetworkForwardPfDefPtr def)
 }
 
 
+static void
+virNetworkDHCPLeaseTimeDefClear(virNetworkDHCPLeaseTimeDefPtr lease)
+{
+VIR_FREE(lease);
+}
+
+
 static void
 virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def)
 {
 VIR_FREE(def->mac);
 VIR_FREE(def->id);
 VIR_FREE(def->name);
+VIR_FREE(def->lease);
 }
 
 
@@ -145,6 +160,9 @@ static void
 virNetworkIPDefClear(virNetworkIPDefPtr def)
 {
 VIR_FREE(def->family);
+
+while (def->nranges)
+virNetworkDHCPLeaseTimeDefClear(def->ranges[--def->nranges].lease);
 VIR_FREE(def->ranges);
 
 while (def->nhosts)
@@ -391,11 +409,55 @@ int virNetworkIPDefNetmask(const virNetworkIPDef *def,
 
 
 static int
-virSocketAddrRangeParseXML(const char *networkName,
-   virNetworkIPDefPtr ipdef,
-   xmlNodePtr node,
-   virSocketAddrRangePtr range)
+virNetworkDHCPLeaseTimeDefParseXML(virNetworkDHCPLeaseTimeDefPtr *lease,
+   xmlNodePtr node)
+{
+virNetworkDHCPLeaseTimeDefPtr new_lease = *lease;
+g_autofree char *expiry = NULL, *unit = NULL;
+
+if (!(expiry = virXMLPropString(node, "expiry")))
+return 0;
+
+if (VIR_ALLOC(new_lease) < 0)
+return -1;
+
+if (virStrToLong_ul(expiry, NULL, 10, &new_lease->expiry) < 0)
+return -1;
+
+if (!(unit = virXMLPropString(node, "unit")))
+new_lease->unit = VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES;
+else
+new_lease->unit = virNetworkDHCPLeaseTimeUnitTypeFromString(unit);
+
+/* infinite */
+if (new_lease->expiry > 0) {
+/* This boundary check is related to dnsmasq man page settings:
+ * "The minimum lease time is two minutes." */
+if ((new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS &&
+ new_lease->expiry < 120) ||
+(new_lease->unit == VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES &&
+ new_lease->expiry < 2)) {
+virReportError(VIR_ERR_CON

[PATCH v3 2/2] tests: Add tests for to cover dnsmasq settings

[Julio Faracco]

New tests are required to cover some new XML syntax entry for
 option. This includes schema testing and other features
like unit attribute and lease value. This commit includes hostsfile
checks adding new files for each test case that is manipulating 
tag.

Signed-off-by: Julio Faracco 
---
 .../dhcp6-nat-network.hostsfile   |  7 +
 .../dhcp6-network.hostsfile   |  5 
 .../dhcp6host-routed-network.hostsfile|  7 +
 .../networkxml2confdata/leasetime-hours.conf  | 16 +++
 .../leasetime-hours.hostsfile |  2 ++
 tests/networkxml2confdata/leasetime-hours.xml | 19 +
 .../leasetime-infinite.conf   | 16 +++
 .../leasetime-infinite.hostsfile  |  2 ++
 .../leasetime-infinite.xml| 19 +
 .../leasetime-minutes.conf| 16 +++
 .../leasetime-minutes.hostsfile   |  2 ++
 .../networkxml2confdata/leasetime-minutes.xml | 19 +
 .../leasetime-seconds.conf| 16 +++
 .../leasetime-seconds.hostsfile   |  2 ++
 .../networkxml2confdata/leasetime-seconds.xml | 19 +
 ...t-network-dns-srv-record-minimal.hostsfile |  2 ++
 .../nat-network-dns-srv-record.hostsfile  |  2 ++
 .../nat-network-dns-txt-record.hostsfile  |  2 ++
 .../nat-network-mtu.hostsfile |  2 ++
 .../nat-network-name-with-quotes.hostsfile|  2 ++
 .../networkxml2confdata/nat-network.hostsfile |  2 ++
 .../ptr-domains-auto.hostsfile|  2 ++
 tests/networkxml2conftest.c   | 27 +--
 tests/networkxml2xmlin/leasetime-hours.xml| 19 +
 tests/networkxml2xmlin/leasetime-infinite.xml | 19 +
 tests/networkxml2xmlin/leasetime-minutes.xml  | 19 +
 tests/networkxml2xmlin/leasetime-seconds.xml  | 19 +
 tests/networkxml2xmlout/leasetime-hours.xml   | 21 +++
 .../networkxml2xmlout/leasetime-infinite.xml  | 21 +++
 tests/networkxml2xmlout/leasetime-minutes.xml | 21 +++
 tests/networkxml2xmlout/leasetime-seconds.xml | 21 +++
 tests/networkxml2xmltest.c|  4 +++
 32 files changed, 370 insertions(+), 2 deletions(-)
 create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile
 create mode 100644 tests/networkxml2confdata/nat-network-mtu.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile
 create mode 100644 tests/networkxml2confdata/nat-network.hostsfile
 create mode 100644 tests/networkxml2confdata/ptr-domains-auto.hostsfile
 create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml

diff --git a/tests/networkxml2confdata/dhcp6-nat-network.hostsfile 
b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile
new file mode 100644
index 00..de659b98c5
--- /dev/null
+++ b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile
@@ -0,0 +1,7 @@
+00:16:3e:77:e2:ed,192.168.122.10,a.example.com
+00:16:3e:3e:a9:1a,192.168.122.11,b.example.com
+id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20]
+paul,[2001:db8:ac10:fd01::1:21]
+id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22]
+i

[PATCH v3 0/2] Include lease time option into DHCP settings

[Julio Faracco]

This series is based on latest series from Nehal. It includes a new
entry called  under  and  from  scope.
This was implemented to include independent lease time for each line and
dnsmasq option. So, users are able to define one lease time for ranges
and other different for each host entry. The new syntax is simlar with:


  

  
  

  


It will produce a option in dnsmasq configuration file:
dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,14m

And some contents into hostsfile:
00:16:3e:77:e2:ed,192.168.122.10,a.example.com,1h

This series includes some test cases to cover lease time XML syntax
also. Now, each test case requires a hostsfile to test this specific
setting.

- v1-v2: Change XML syntax according Daniel's suggestion.
- v2-v3: Fix memory leak and test dependency issue.

Julio Faracco (2):
  conf: Add  option for  settings
  tests: Add tests for  to cover dnsmasq settings

 docs/schemas/basictypes.rng   |   8 +
 docs/schemas/network.rng  |  20 +++
 src/conf/network_conf.c   | 159 +++---
 src/conf/network_conf.h   |  27 ++-
 src/libvirt_private.syms  |   3 +
 src/network/bridge_driver.c   |  56 +-
 src/network/bridge_driver.h   |   1 +
 src/test/test_driver.c|   2 +-
 src/util/virdnsmasq.c |  60 ---
 src/util/virdnsmasq.h |   3 +
 src/vbox/vbox_network.c   |  16 +-
 .../dhcp6-nat-network.hostsfile   |   7 +
 .../dhcp6-network.hostsfile   |   5 +
 .../dhcp6host-routed-network.hostsfile|   7 +
 .../networkxml2confdata/leasetime-hours.conf  |  16 ++
 .../leasetime-hours.hostsfile |   2 +
 tests/networkxml2confdata/leasetime-hours.xml |  19 +++
 .../leasetime-infinite.conf   |  16 ++
 .../leasetime-infinite.hostsfile  |   2 +
 .../leasetime-infinite.xml|  19 +++
 .../leasetime-minutes.conf|  16 ++
 .../leasetime-minutes.hostsfile   |   2 +
 .../networkxml2confdata/leasetime-minutes.xml |  19 +++
 .../leasetime-seconds.conf|  16 ++
 .../leasetime-seconds.hostsfile   |   2 +
 .../networkxml2confdata/leasetime-seconds.xml |  19 +++
 ...t-network-dns-srv-record-minimal.hostsfile |   2 +
 .../nat-network-dns-srv-record.hostsfile  |   2 +
 .../nat-network-dns-txt-record.hostsfile  |   2 +
 .../nat-network-mtu.hostsfile |   2 +
 .../nat-network-name-with-quotes.hostsfile|   2 +
 .../networkxml2confdata/nat-network.hostsfile |   2 +
 .../ptr-domains-auto.hostsfile|   2 +
 tests/networkxml2conftest.c   |  42 -
 tests/networkxml2xmlin/leasetime-hours.xml|  19 +++
 tests/networkxml2xmlin/leasetime-infinite.xml |  19 +++
 tests/networkxml2xmlin/leasetime-minutes.xml  |  19 +++
 tests/networkxml2xmlin/leasetime-seconds.xml  |  19 +++
 tests/networkxml2xmlout/leasetime-hours.xml   |  21 +++
 .../networkxml2xmlout/leasetime-infinite.xml  |  21 +++
 tests/networkxml2xmlout/leasetime-minutes.xml |  21 +++
 tests/networkxml2xmlout/leasetime-seconds.xml |  21 +++
 tests/networkxml2xmltest.c|   4 +
 43 files changed, 676 insertions(+), 66 deletions(-)
 create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile
 create mode 100644 tests/networkxml2confdata/nat-network-mtu.hostsfile
 create mode 100644 
tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile
 create mode 100644 tests/networkxml2confdata/nat-network.hostsfile
 create mode 10064

Re: [PATCH v2 1/2] conf: Add option for settings

[Julio Faracco]

Em qua., 22 de abr. de 2020 às 10:45, Daniel Henrique Barboza
 escreveu:
>
>
>
> On 4/22/20 9:22 AM, Julio Faracco wrote:
> > Hi Daniel,
> >
> > Thanks for reviewing. :-)
>
>
> Np, glad to be of assistance
>
>
> > IMHO, I don't like to join them in one single patch because it is hard
> > to review.
>
>
> Hmm, reading this I believe that the tests would pass after applying patch
> 2/2 then (didn't try).
>
> > I know that others have a different opinion (and it is good).
>
>
> The way you split the patch series is up to debate and so on and so
> forth. The issue here is that every single patch must pass 'make check',
> as said in docs/hacking.rst in the "Preparing patches" session:
>
> --
> If you're going to submit multiple patches, the automated tests
> must pass **after each patch**, not just after the last one.
> --

Yes, I usually keep a post commit hook, but I (unfortunately) moved to
another Linux distro without my customization. Sorry about that.

There is a memory leak also. I need to resend this series anyway.

>
> You'll need to think in a way of splitting the series that allows each
> patch to pass 'make check' in its own.
>
>
> Thanks,
>
>
> DHB
>

Re: [PATCH] news: Document new Xen hypervisor features

[Jim Fehlig]

On 4/22/20 11:06 AM, Andrea Bolognani wrote:

On Wed, 2020-04-22 at 10:43 -0600, Jim Fehlig wrote:

On 4/22/20 9:10 AM, Andrea Bolognani wrote:

If you look at most (all?) other entries, they look like

subsystem: Change

which is the same style we use for commit messages.


If you really want to split hairs, the subsytem is 'libxl'. But I don't think
that is good for news since in general folks know of the 'Xen' project, but not
'libxl'. However, now that the old xen driver is removed libxl could be renamed
to xen :-).


Yeah, I know of the history :) And I agree that we should probably
rename the driver to xen[1]; as you probably remember, the standalone
daemon is called virtxend and not virtlibxld already :)


Xen seems to be the outlier when it comes to following this style,
both in release notes and to some extent in commit messages. It would
be great if it could fall in line with the rest of the code base.


Sorry for being out of line. I'll try to be more conscious of that in the 
future.


Nothing to apologize for! I just pointed it out so that we can, in
time, get closer to a state of consistency :)


[1] Although it will never be a full rename, because it still need to
 handle libxl:// connection URLs.


libxl:// was never supported. DV rightly pointed out that introducing libxl:// 
as another way to connect to Xen violated one of libvirt's core principles of 
"minimize the change on the application stack as the lower layers of

virtualization evolves". It took me a while to find that bit of history :-)

https://www.redhat.com/archives/libvir-list/2011-March/msg00449.html

You'll also notice libxl:// is not mentioned on the Connect URI page

https://libvirt.org/uri.html

Regards,
Jim

Re: [all PATCH] gitlab: add CI job for validating DCO signoff

[Andrea Bolognani]

On Wed, 2020-04-22 at 18:06 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 07:01:50PM +0200, Andrea Bolognani wrote:
> > I still don't understand why we would want to single out those
> > branches and not run the DCO check on them. What harm would it
> > cause? It takes around a minute to run it, which is significantly
> > less than the other jobs running during the prebuild stage...
> 
> The check-dco script doesn't actually work if run against the
> main libvirt repo, as it ends up trying to use itself as a
> reference and failing to figure out which commits need checking.
> Of course that's a bug that's fixable, but in general I think it
> is better to not runthe job at all and thus eliminate any risk
> of false failures.

It seems to work fine here:

  https://gitlab.com/abologna/libvirt/-/jobs/522467078

The corresponding commit is

  
https://gitlab.com/abologna/libvirt/-/commit/435ef06536f590f4403d0ce59a1b9edc1dfa80ec

where I changed the local require-dco.py script to use the very same
branch I pushed to as the base branch for DCO checking.

I believe you fixed the issue you mention above with

  commit 769ff77c9c5afaec97350a4931e5ca123b6af6d2
  Author: Daniel P. Berrangé 
  Date:   Fri Mar 27 14:38:49 2020 +

scripts: avoid error in DCO check on empty branches

If the DCO check is run on an empty branch (ie one which has no commits
different from master), it throws an error due to trying to interpret
the empty string as a git commit SHA.

Reviewed-by: Andrea Bolognani 
Signed-off-by: Daniel P. Berrangé 

  v6.1.0-306-g769ff77c9c

So I see no reason not to just run the check on all branches.


If you remove the except: part, and of course the corresponding
comment as well, then

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

[all PATCH v2] gitlab: add CI job for validating DCO signoff

[Daniel P . Berrangé]

This job uses the shared "check-dco" image to validate that all
commits on a branch in a developer's repo fork have a suitable
Signed-off-by statement present.

Signed-off-by: Daniel P. Berrangé 
---

This patch was against the Perl repo, but if this is approved then
I'll also apply it to *all* the other repos which currently lack a
DCO check, without reposting further patches for each repo.

For libvirt.git I'll send a patch to update its existing DCO job.

Changed in v2:

 - Exclude based on project namespace, not branch names

.gitlab-ci.yml | 16 
 1 file changed, 16 insertions(+)
 create mode 100644 .gitlab-ci.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 000..50dae92
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,16 @@
+
+stages:
+  - prebuild
+
+# Check that all commits are signed-off for the DCO.
+# Skip on "libvirt" namespace, since we only need to run
+# this test on developer's personal forks from which
+# merge requests are submitted
+check-dco:
+  stage: prebuild
+  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
+  script:
+- /check-dco
+  except:
+variables:
+  - $CI_PROJECT_NAMESPACE == 'libvirt'
-- 
2.25.2

Re: [PATCH] news: Document new Xen hypervisor features

[Andrea Bolognani]

On Wed, 2020-04-22 at 10:43 -0600, Jim Fehlig wrote:
> On 4/22/20 9:10 AM, Andrea Bolognani wrote:
> > If you look at most (all?) other entries, they look like
> > 
> >subsystem: Change
> > 
> > which is the same style we use for commit messages.
> 
> If you really want to split hairs, the subsytem is 'libxl'. But I don't think 
> that is good for news since in general folks know of the 'Xen' project, but 
> not 
> 'libxl'. However, now that the old xen driver is removed libxl could be 
> renamed 
> to xen :-).

Yeah, I know of the history :) And I agree that we should probably
rename the driver to xen[1]; as you probably remember, the standalone
daemon is called virtxend and not virtlibxld already :)

> > Xen seems to be the outlier when it comes to following this style,
> > both in release notes and to some extent in commit messages. It would
> > be great if it could fall in line with the rest of the code base.
> 
> Sorry for being out of line. I'll try to be more conscious of that in the 
> future.

Nothing to apologize for! I just pointed it out so that we can, in
time, get closer to a state of consistency :)


[1] Although it will never be a full rename, because it still need to
handle libxl:// connection URLs.
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [all PATCH] gitlab: add CI job for validating DCO signoff

[Daniel P . Berrangé]

On Wed, Apr 22, 2020 at 07:01:50PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 17:20 +0100, Daniel P. Berrangé wrote:
> > On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> > > Why is it that we want to skip those branches, anyway? I get why
> > > they're not necessary in a MR-based workflow, but we're not quite
> > > there yet...
> > 
> > This was an inexact way to stop the checks running against the
> > master repo, after the patches have been merged.
> > 
> > The flaw in this is that a user could indeed open a merge request
> > that uses a "master" or "v*maint" branch in their private fork,
> > rather than a named feature branch.
> > 
> > Really we want it to run on all commits in a user's fork, but
> > not run in the master repos post-merge.
> 
> I still don't understand why we would want to single out those
> branches and not run the DCO check on them. What harm would it
> cause? It takes around a minute to run it, which is significantly
> less than the other jobs running during the prebuild stage...

The check-dco script doesn't actually work if run against the
main libvirt repo, as it ends up trying to use itself as a
reference and failing to figure out which commits need checking.
Of course that's a bug that's fixable, but in general I think it
is better to not runthe job at all and thus eliminate any risk
of false failures.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [all PATCH] gitlab: add CI job for validating DCO signoff

[Andrea Bolognani]

On Wed, 2020-04-22 at 17:20 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> > Why is it that we want to skip those branches, anyway? I get why
> > they're not necessary in a MR-based workflow, but we're not quite
> > there yet...
> 
> This was an inexact way to stop the checks running against the
> master repo, after the patches have been merged.
> 
> The flaw in this is that a user could indeed open a merge request
> that uses a "master" or "v*maint" branch in their private fork,
> rather than a named feature branch.
> 
> Really we want it to run on all commits in a user's fork, but
> not run in the master repos post-merge.

I still don't understand why we would want to single out those
branches and not run the DCO check on them. What harm would it
cause? It takes around a minute to run it, which is significantly
less than the other jobs running during the prebuild stage...

> > Actually, now that we're using GitLab as the primary repository,
> > how are we ensuring commits without DCO don't slip in? We had a
> > hook that took care of that on libvirt.org - was something like
> > that introduced on GitLab?
> 
> It isn't as strict as before - there's a push rule that requires
> the word "Signed-off-by" in the commit message:
> 
>   https://libvirt.org/newreposetup.html

Oh, cool! I had forgotten about that detail since reviewing the
document, and it's nice to know that we still have at least some
level of protection on that front :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] docs: drvqemu: trivial fix for qemu commands passthrough

[Ján Tomko]

On a Wednesday in 2020, Chen Hanxiao wrote:

element  should be the child of 

Signed-off-by: Chen Hanxiao 
---
docs/drvqemu.html.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH 4/5] util: remove references to virRun/virExec

[Ján Tomko]

virCommand is now used everywhere.

Signed-off-by: Ján Tomko 
Suggested-by: Sebastian Mitterle 
---
 src/util/virprocess.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b126c3c9af..afb1f9b79f 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -129,7 +129,7 @@ VIR_ENUM_IMPL(virProcessSchedPolicy,
  * @status: child exit status to translate
  *
  * Translate an exit status into a malloc'd string.  Generic helper
- * for virCommandRun(), virCommandWait(), virRun(), and virProcessWait()
+ * for virCommandRun(), virCommandWait() and virProcessWait()
  * status argument, as well as raw waitpid().
  */
 char *
@@ -1190,7 +1190,7 @@ virProcessRunInForkHelper(int errfd,
  * particular no mutexes should be used in @cb, unless steps were
  * taken before forking to guarantee a predictable state. @cb
  * must not exec any external binaries, instead
- * virCommand/virExec should be used for that purpose.
+ * virCommand should be used for that purpose.
  *
  * On return, the returned value is either -1 with error message
  * reported if something went bad in the parent, if child has
-- 
2.25.1

Re: [PATCH 6/6] qemuDomainSaveImageOpen: Refactor handling of errors

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

Return error codes directly and fix weird reporting of errors via
temporary variable.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 51 +-
1 file changed, 26 insertions(+), 25 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 5/6] qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N)

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH 1/5] openvz: switch from virRun to virCommand

[Ján Tomko]

Construct the command in multiple steps instead of using a sentinel
in the args array.

Signed-off-by: Ján Tomko 
---
 src/openvz/openvz_driver.c | 96 +-
 1 file changed, 42 insertions(+), 54 deletions(-)

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 1a189dbbe7..0a08c63b1b 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -481,30 +481,11 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, 
unsigned int flags) {
 }
 
 
-/*
- * Convenient helper to target a command line argv
- * and fill in an empty slot with the supplied
- * key value. This lets us declare the argv on the
- * stack and just splice in the domain name after
- */
-#define PROGRAM_SENTINEL ((char *)0x1)
-static void openvzSetProgramSentinal(const char **prog, const char *key)
-{
-const char **tmp = prog;
-while (tmp && *tmp) {
-if (*tmp == PROGRAM_SENTINEL) {
-*tmp = key;
-break;
-}
-tmp++;
-}
-}
-
 static int openvzDomainSuspend(virDomainPtr dom)
 {
+g_autoptr(virCommand) cmd = virCommandNewArgList(VZCTL, "--quiet", 
"chkpnt", NULL);
 struct openvz_driver *driver = dom->conn->privateData;
 virDomainObjPtr vm;
-const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, 
"--suspend", NULL};
 int ret = -1;
 
 if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
@@ -514,8 +495,8 @@ static int openvzDomainSuspend(virDomainPtr dom)
 goto cleanup;
 
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
-openvzSetProgramSentinal(prog, vm->def->name);
-if (virRun(prog, NULL) < 0)
+virCommandAddArgList(cmd, vm->def->name, "--suspend", NULL);
+if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
 virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
 }
@@ -529,9 +510,9 @@ static int openvzDomainSuspend(virDomainPtr dom)
 
 static int openvzDomainResume(virDomainPtr dom)
 {
+g_autoptr(virCommand) cmd = virCommandNewArgList(VZCTL, "--quiet", 
"chkpnt", NULL);
 struct openvz_driver *driver = dom->conn->privateData;
 virDomainObjPtr vm;
-const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, 
"--resume", NULL};
 int ret = -1;
 
 if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
@@ -541,8 +522,8 @@ static int openvzDomainResume(virDomainPtr dom)
 goto cleanup;
 
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-openvzSetProgramSentinal(prog, vm->def->name);
-if (virRun(prog, NULL) < 0)
+virCommandAddArgList(cmd, vm->def->name, "--resume", NULL);
+if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
 virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, 
VIR_DOMAIN_RUNNING_UNPAUSED);
 }
@@ -558,9 +539,9 @@ static int
 openvzDomainShutdownFlags(virDomainPtr dom,
   unsigned int flags)
 {
+g_autoptr(virCommand) cmd = virCommandNewArgList(VZCTL, "--quiet", "stop", 
NULL);
 struct openvz_driver *driver = dom->conn->privateData;
 virDomainObjPtr vm;
-const char *prog[] = {VZCTL, "--quiet", "stop", PROGRAM_SENTINEL, NULL};
 int ret = -1;
 int status;
 
@@ -572,14 +553,15 @@ openvzDomainShutdownFlags(virDomainPtr dom,
 if (openvzGetVEStatus(vm, &status, NULL) == -1)
 goto cleanup;
 
-openvzSetProgramSentinal(prog, vm->def->name);
 if (status != VIR_DOMAIN_RUNNING) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domain is not in running state"));
 goto cleanup;
 }
 
-if (virRun(prog, NULL) < 0)
+virCommandAddArg(cmd, vm->def->name);
+
+if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
 
 vm->def->id = -1;
@@ -613,9 +595,9 @@ openvzDomainDestroyFlags(virDomainPtr dom, unsigned int 
flags)
 static int openvzDomainReboot(virDomainPtr dom,
   unsigned int flags)
 {
+g_autoptr(virCommand) cmd = virCommandNewArgList(VZCTL, "--quiet", 
"restart", NULL);
 struct openvz_driver *driver = dom->conn->privateData;
 virDomainObjPtr vm;
-const char *prog[] = {VZCTL, "--quiet", "restart", PROGRAM_SENTINEL, NULL};
 int ret = -1;
 int status;
 
@@ -627,15 +609,16 @@ static int openvzDomainReboot(virDomainPtr dom,
 if (openvzGetVEStatus(vm, &status, NULL) == -1)
 goto cleanup;
 
-openvzSetProgramSentinal(prog, vm->def->name);
 if (status != VIR_DOMAIN_RUNNING) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domain is not in running state"));
 goto cleanup;
 }
 
-if (virRun(prog, NULL) < 0)
+virCommandAddArg(cmd, vm->def->name);
+if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
+
 ret = 0;
 
 virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
@@ -935,11 +918,11 @@ static virDomainPtr
 openvzDomainCreateXML(vi

[libvirt PATCH 3/5] Remove all usage of virRun

[Ján Tomko]

Catch the individual usage not removed in previous commits.

Signed-off-by: Ján Tomko 
---
 src/lxc/lxc_driver.c |  5 +++--
 src/qemu/qemu_domain.c   | 16 
 src/security/security_apparmor.c | 11 +++
 src/util/virnetdev.c | 24 
 4 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 851894c459..61dd35360a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1423,10 +1423,11 @@ lxcDomainDestroy(virDomainPtr dom)
 
 static int lxcCheckNetNsSupport(void)
 {
-const char *argv[] = {"ip", "link", "set", "lo", "netns", "-1", NULL};
+g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link", "set", "lo",
+ "netns", "-1", NULL);
 int ip_rc;
 
-if (virRun(argv, &ip_rc) < 0 || ip_rc == 255)
+if (virCommandRun(cmd, &ip_rc) < 0 || ip_rc == 255)
 return 0;
 
 if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_NET) < 0)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 98ffd23a71..3d075bca26 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7553,20 +7553,20 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriverPtr 
driver,
   bool try_all,
   int ndisks)
 {
-const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
+const char *qemuimgbin;
 size_t i;
 bool skipped = false;
 
-qemuimgarg[0] = qemuFindQemuImgBinary(driver);
-if (qemuimgarg[0] == NULL) {
+qemuimgbin = qemuFindQemuImgBinary(driver);
+if (qemuimgbin == NULL) {
 /* qemuFindQemuImgBinary set the error */
 return -1;
 }
 
-qemuimgarg[2] = op;
-qemuimgarg[3] = name;
-
 for (i = 0; i < ndisks; i++) {
+g_autoptr(virCommand) cmd = virCommandNewArgList(qemuimgbin, 
"snapshot",
+ op, name, NULL);
+
 /* FIXME: we also need to handle LVM here */
 if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
 int format = virDomainDiskGetFormat(def->disks[i]);
@@ -7593,9 +7593,9 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriverPtr driver,
 return -1;
 }
 
-qemuimgarg[4] = virDomainDiskGetSource(def->disks[i]);
+virCommandAddArg(cmd, virDomainDiskGetSource(def->disks[i]));
 
-if (virRun(qemuimgarg, NULL) < 0) {
+if (virCommandRun(cmd, NULL) < 0) {
 if (try_all) {
 VIR_WARN("skipping snapshot action on %s",
  def->disks[i]->dst);
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index ca02631f7f..55eb110522 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -203,15 +203,10 @@ load_profile(virSecurityManagerPtr mgr G_GNUC_UNUSED,
 static int
 remove_profile(const char *profile)
 {
-int rc = -1;
-const char * const argv[] = {
-VIRT_AA_HELPER, "-D", "-u", profile, NULL
-};
+g_autoptr(virCommand) cmd = virCommandArgList(VIRT_AA_HELPER, "-D", "-u",
+  profile, NULL);
 
-if (virRun(argv, NULL) == 0)
-rc = 0;
-
-return rc;
+return virCommandRun(cmd, NULL);
 }
 
 static char *
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index dea0bccd57..1d024b8b97 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -508,6 +508,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
 g_autofree char *pid = NULL;
 g_autofree char *phy = NULL;
 g_autofree char *phy_path = NULL;
+g_autoptr(virCommand) cmd = NULL;
 int len;
 
 pid = g_strdup_printf("%lld", (long long) pidInNs);
@@ -518,28 +519,19 @@ int virNetDevSetNamespace(const char *ifname, pid_t 
pidInNs)
 
 if ((len = virFileReadAllQuiet(phy_path, 1024, &phy)) <= 0) {
 /* Not a wireless device. */
-const char *argv[] = {
-"ip", "link", "set", ifname, "netns", NULL, NULL
-};
-
-argv[5] = pid;
-if (virRun(argv, NULL) < 0)
-return -1;
-
+cmd = virCommandNewArgList("ip", "link",
+   "set", ifname, "netns", pid, NULL);
 } else {
-const char *argv[] = {
-"iw", "phy", NULL, "set", "netns", NULL, NULL
-};
-
 /* Remove a line break. */
 phy[len - 1] = '\0';
 
-argv[2] = phy;
-argv[5] = pid;
-if (virRun(argv, NULL) < 0)
-return -1;
+cmd = virCommandNewArgList("iw", "phy", phy,
+   "set", "netns", pid, NULL);
 }
 
+if (virCommandRun(cmd, NULL) < 0)
+return -1;
+
 return 0;
 }
 
-- 
2.25.1

[libvirt PATCH 5/5] util: remove virRun

[Ján Tomko]

Everything is using virCommand now.

Signed-off-by: Ján Tomko 
---
 src/libvirt_private.syms |  1 -
 src/util/vircommand.c| 35 ---
 src/util/vircommand.h|  2 --
 3 files changed, 38 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 28610a837e..7f497898eb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1874,7 +1874,6 @@ virCommandToString;
 virCommandWait;
 virCommandWriteArgLog;
 virFork;
-virRun;
 
 
 # util/virconf.h
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index b84fb40948..20f196104f 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -893,43 +893,8 @@ virExec(virCommandPtr cmd)
 }
 
 
-/**
- * virRun:
- * @argv NULL terminated argv to run
- * @status optional variable to return exit status in
- *
- * Run a command without using the shell.
- *
- * If status is NULL, then return 0 if the command run and
- * exited with 0 status; Otherwise return -1
- *
- * If status is not-NULL, then return 0 if the command ran.
- * The status variable is filled with the command exit status
- * and should be checked by caller for success. Return -1
- * only if the command could not be run.
- */
-int
-virRun(const char *const*argv, int *status)
-{
-g_autoptr(virCommand) cmd = virCommandNewArgs(argv);
-
-return virCommandRun(cmd, status);
-}
-
 #else /* WIN32 */
 
-int
-virRun(const char *const *argv G_GNUC_UNUSED,
-   int *status)
-{
-if (status)
-*status = ENOTSUP;
-else
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("virRun is not implemented for WIN32"));
-return -1;
-}
-
 pid_t
 virFork(void)
 {
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index 9086f9a90c..e2be5bcf1c 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -34,8 +34,6 @@ typedef int (*virExecHook)(void *data);
 
 pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT;
 
-int virRun(const char *const*argv, int *status) G_GNUC_WARN_UNUSED_RESULT;
-
 virCommandPtr virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
 
 virCommandPtr virCommandNewArgs(const char *const*args) ATTRIBUTE_NONNULL(1);
-- 
2.25.1

Re: [PATCH 4/6] qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

Use VIR_AUTOCLOSE to declare it and remove all internal closing of the
filedescriptor. This will allow getting rid of 'error' completely.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH 0/5] Remove virRun in favor of virCommand

[Ján Tomko]

Ján Tomko (5):
  openvz: switch from virRun to virCommand
  vmware: use virCommand instead of virRun
  Remove all usage of virRun
  util: remove references to virRun/virExec
  util: remove virRun

 src/libvirt_private.syms |  1 -
 src/lxc/lxc_driver.c |  5 +-
 src/openvz/openvz_driver.c   | 96 ++--
 src/qemu/qemu_domain.c   | 16 +++---
 src/security/security_apparmor.c | 11 +---
 src/util/vircommand.c| 35 
 src/util/vircommand.h|  2 -
 src/util/virnetdev.c | 24 +++-
 src/util/virprocess.c|  4 +-
 src/vmware/vmware_conf.c |  9 ++-
 src/vmware/vmware_driver.c   | 69 ++-
 11 files changed, 98 insertions(+), 174 deletions(-)

-- 
2.25.1

[libvirt PATCH 2/5] vmware: use virCommand instead of virRun

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/vmware/vmware_conf.c   |  9 +++--
 src/vmware/vmware_driver.c | 69 --
 2 files changed, 32 insertions(+), 46 deletions(-)

diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
index fd62bb96f7..8bea9c3b12 100644
--- a/src/vmware/vmware_conf.c
+++ b/src/vmware/vmware_conf.c
@@ -436,8 +436,7 @@ vmwareVmxPath(virDomainDefPtr vmdef, char **vmxPath)
 int
 vmwareMoveFile(char *srcFile, char *dstFile)
 {
-const char *cmdmv[] =
-{ "mv", PROGRAM_SENTINEL, PROGRAM_SENTINEL, NULL };
+g_autoptr(virCommand) cmd = NULL;
 
 if (!virFileExists(srcFile)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, _("file %s does not exist"),
@@ -448,9 +447,9 @@ vmwareMoveFile(char *srcFile, char *dstFile)
 if (STREQ(srcFile, dstFile))
 return 0;
 
-vmwareSetSentinal(cmdmv, srcFile);
-vmwareSetSentinal(cmdmv, dstFile);
-if (virRun(cmdmv, NULL) < 0) {
+cmd = virCommandNewArgList("mv", srcFile, dstFile, NULL);
+
+if (virCommandRun(cmd, NULL) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to move file to %s "), dstFile);
 return -1;
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index d5dd6e4f5e..5b4057a8f6 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -347,15 +347,12 @@ vmwareStopVM(struct vmware_driver *driver,
  virDomainObjPtr vm,
  virDomainShutoffReason reason)
 {
-const char *cmd[] = {
-driver->vmrun, "-T", PROGRAM_SENTINEL, "stop",
-PROGRAM_SENTINEL, "soft", NULL
-};
+g_autoptr(virCommand) cmd = virCommandNew(driver->vmrun);
 
-vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type));
-vmwareSetSentinal(cmd, ((vmwareDomainPtr) vm->privateData)->vmxPath);
+virCommandAddArgList(cmd, "-T", vmwareDriverTypeToString(driver->type),
+"stop", ((vmwareDomainPtr) vm->privateData)->vmxPath, "soft", NULL);
 
-if (virRun(cmd, NULL) < 0)
+if (virCommandRun(cmd, NULL) < 0)
 return -1;
 
 vm->def->id = -1;
@@ -367,26 +364,22 @@ vmwareStopVM(struct vmware_driver *driver,
 static int
 vmwareStartVM(struct vmware_driver *driver, virDomainObjPtr vm)
 {
-const char *cmd[] = {
-driver->vmrun, "-T", PROGRAM_SENTINEL, "start",
-PROGRAM_SENTINEL, PROGRAM_SENTINEL, NULL
-};
+g_autoptr(virCommand) cmd = virCommandNew(driver->vmrun);
 const char *vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath;
 
+virCommandAddArgList(cmd, "-T", vmwareDriverTypeToString(driver->type),
+ "start", vmxPath, NULL);
+
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("domain is not in shutoff state"));
 return -1;
 }
 
-vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type));
-vmwareSetSentinal(cmd, vmxPath);
 if (!((vmwareDomainPtr) vm->privateData)->gui)
-vmwareSetSentinal(cmd, NOGUI);
-else
-vmwareSetSentinal(cmd, NULL);
+virCommandAddArg(cmd, NOGUI);
 
-if (virRun(cmd, NULL) < 0)
+if (virCommandRun(cmd, NULL) < 0)
 return -1;
 
 if ((vm->def->id = vmwareExtractPid(vmxPath)) < 0) {
@@ -543,12 +536,9 @@ static int
 vmwareDomainSuspend(virDomainPtr dom)
 {
 struct vmware_driver *driver = dom->conn->privateData;
+g_autoptr(virCommand) cmd = virCommandNew(driver->vmrun);
 
 virDomainObjPtr vm;
-const char *cmd[] = {
-  driver->vmrun, "-T", PROGRAM_SENTINEL, "pause",
-  PROGRAM_SENTINEL, NULL
-};
 int ret = -1;
 
 if (driver->type == VMWARE_DRIVER_PLAYER) {
@@ -561,15 +551,17 @@ vmwareDomainSuspend(virDomainPtr dom)
 if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid)))
 return -1;
 
-vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type));
-vmwareSetSentinal(cmd, ((vmwareDomainPtr) vm->privateData)->vmxPath);
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domain is not in running state"));
 goto cleanup;
 }
 
-if (virRun(cmd, NULL) < 0)
+virCommandAddArgList(cmd, "-T", vmwareDriverTypeToString(driver->type),
+ "pause", ((vmwareDomainPtr) vm->privateData)->vmxPath,
+ NULL);
+
+if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
 
 virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
@@ -584,12 +576,9 @@ static int
 vmwareDomainResume(virDomainPtr dom)
 {
 struct vmware_driver *driver = dom->conn->privateData;
+g_autoptr(virCommand) cmd = virCommandNew(driver->vmrun);
 
 virDomainObjPtr vm;
-const char *cmd[] = {
-driver->vmrun, "-T", PROGRAM_SENTINEL, "unpause", PROGRAM_SENTINEL,
-NULL
-};
 int ret = -1;
 
 if (dr

Re: [PATCH 3/6] qemuDomainSaveImageOpen: Use g_autoptr for 'dom'

[Ján Tomko]

s/dom/def in the commit message

On a Wednesday in 2020, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)



Proofread-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/6] virQEMUSaveData: Register autoclear function and use it in qemuDomainSaveImageOpen

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

In an attempt to simplify qemuDomainSaveImageOpen we need to add
automatic pointer clearing for virQEMUSaveData.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] news: Document new Xen hypervisor features

[Jim Fehlig]

On 4/22/20 9:10 AM, Andrea Bolognani wrote:

On Wed, 2020-04-22 at 08:48 -0600, Jim Fehlig wrote:

On 4/22/20 1:51 AM, Andrea Bolognani wrote:

On Tue, 2020-04-21 at 16:06 -0600, Jim Fehlig wrote:

+
+  Xen: add support for e820_host hypervisor feature


s/Xen: add/xen: Add/


There is only one 'xen:' in news, all other instances are 'Xen:'. I should stick
with the majority.


If you look at most (all?) other entries, they look like

   subsystem: Change

which is the same style we use for commit messages.


If you really want to split hairs, the subsytem is 'libxl'. But I don't think 
that is good for news since in general folks know of the 'Xen' project, but not 
'libxl'. However, now that the old xen driver is removed libxl could be renamed 
to xen :-).



Xen seems to be the outlier when it comes to following this style,
both in release notes and to some extent in commit messages. It would
be great if it could fall in line with the rest of the code base.


Sorry for being out of line. I'll try to be more conscious of that in the 
future.

Regards,
Jim

Re: [PATCH 1/6] qemu: fix domain start with corrupted save file

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

Commit 21ad56e932 introduced a regression where a VM with a corrupted
save image file would fail to start on the first attempt. This was
caused by returning a wrong return code as  'fd' was abused to also hold


double space


the return code.

Since it's easy to miss this nuance introduce a 'ret' variable for the


missing comma


return code and return it's value in the error section.


its



https://bugzilla.redhat.com/show_bug.cgi?id=1791522

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dfe0adaad8..9a9361949d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
bool unlink_corrupt)
{
int fd = -1;
+int ret = -1;
virQEMUSaveDataPtr data = NULL;
virQEMUSaveHeaderPtr header;
virDomainDefPtr def = NULL;


This does not restore the preservation of -errno from the following
call:

if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL)) < 0)
goto error;

that existed before commit 21ad56e932 mentioned above. Thankfully.
Guess it's unlikely that this function would return -ESRCH and confuse
the caller.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/6] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 05:04:54PM +0200, Peter Krempa wrote:
> Commit 21ad56e932 introduced a regression where a VM with a corrupted
> save image file would fail to start on the first attempt. This was
> caused by returning a wrong return code as  'fd' was abused to also hold
> the return code.
> 
> Since it's easy to miss this nuance introduce a 'ret' variable for the
> return code and return it's value in the error section.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> 
> Signed-off-by: Peter Krempa 

Reviewed-by: Pavel Mores 

> ---
>  src/qemu/qemu_driver.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dfe0adaad8..9a9361949d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  bool unlink_corrupt)
>  {
>  int fd = -1;
> +int ret = -1;
>  virQEMUSaveDataPtr data = NULL;
>  virQEMUSaveHeaderPtr header;
>  virDomainDefPtr def = NULL;
> @@ -6726,7 +6727,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>   _("cannot remove corrupt file: %s"),
>   path);
>  } else {
> -fd = -3;
> +ret = -3;
>  }
>  } else {
>  virReportError(VIR_ERR_OPERATION_FAILED,
> @@ -6747,7 +6748,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>   _("cannot remove corrupt file: %s"),
>   path);
>  } else {
> -fd = -3;
> +ret = -3;
>  }
>  goto error;
>  }
> @@ -6816,7 +6817,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  virDomainDefFree(def);
>  virQEMUSaveDataFree(data);
>  VIR_FORCE_CLOSE(fd);
> -return -1;
> +return ret;
>  }
> 
>  static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
> -- 
> 2.26.0
> 

Re: [all PATCH] gitlab: add CI job for validating DCO signoff

[Daniel P . Berrangé]

On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:37 +0100, Daniel P. Berrangé wrote:
> > +# Check that all commits are signed-off for the DCO. Skip
> > +# on master branch and -maint branches, since we only need
> > +# to test developer's personal branches.
> > +check-dco:
> > +  stage: prebuild
> > +  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
> > +  script:
> > +- /check-dco
> > +  only:
> > +- branches
> > +  except:
> > +- master
> 
> You're not actually skipping the -maint branches here, so either
> you need to change this to
> 
>   except:
> - /^v.*-maint$/
> - master
> 
> which libvirt currently uses, or to drop the mention of -maint
> branches from the comment.
> 
> Why is it that we want to skip those branches, anyway? I get why
> they're not necessary in a MR-based workflow, but we're not quite
> there yet...

This was an inexact way to stop the checks running against the
master repo, after the patches have been merged.

The flaw in this is that a user could indeed open a merge request
that uses a "master" or "v*maint" branch in their private fork,
rather than a named feature branch.

Really we want it to run on all commits in a user's fork, but
not run in the master repos post-merge.

> Actually, now that we're using GitLab as the primary repository,
> how are we ensuring commits without DCO don't slip in? We had a
> hook that took care of that on libvirt.org - was something like
> that introduced on GitLab?

It isn't as strict as before - there's a push rule that requires
the word "Signed-off-by" in the commit message:

  https://libvirt.org/newreposetup.html


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [all PATCH] gitlab: add CI job for validating DCO signoff

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:37 +0100, Daniel P. Berrangé wrote:
> +# Check that all commits are signed-off for the DCO. Skip
> +# on master branch and -maint branches, since we only need
> +# to test developer's personal branches.
> +check-dco:
> +  stage: prebuild
> +  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
> +  script:
> +- /check-dco
> +  only:
> +- branches
> +  except:
> +- master

You're not actually skipping the -maint branches here, so either
you need to change this to

  except:
- /^v.*-maint$/
- master

which libvirt currently uses, or to drop the mention of -maint
branches from the comment.

Why is it that we want to skip those branches, anyway? I get why
they're not necessary in a MR-based workflow, but we're not quite
there yet...

Actually, now that we're using GitLab as the primary repository,
how are we ensuring commits without DCO don't slip in? We had a
hook that took care of that on libvirt.org - was something like
that introduced on GitLab?

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH] docs: drvqemu: trivial fix for qemu commands passthrough

[Chen Hanxiao]

element  should be the child of 

Signed-off-by: Chen Hanxiao 
---
 docs/drvqemu.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 5f412ba376..afc4ddf56d 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -578,7 +578,7 @@ mount -t cgroup none /dev/cgroup -o devices
   typically, the namespace is given the name
   of qemu.  With the namespace in place, it is then
   possible to add an element 
-  under driver, with the following sub-elements
+  under domain, with the following sub-elements
   repeated as often as needed:
 
   
-- 
2.23.0

Re: [libvirt PATCH v2] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 05:07:01PM +0200, Peter Krempa wrote:
> On Wed, Apr 22, 2020 at 15:15:31 +0200, Pavel Mores wrote:
> > This is to fix
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> > 
> > With this change, if a domain comes across a corrupted save file during
> > boot it removes the save file and logs a warning but continues to boot
> > normally instead of failing to boot (with a subsequent boot attempt
> > succeeding).
> > 
> > The regression was introduced by 21ad56e932 and this change effectively
> > reverts the relevant part of that commit.
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index e8d47a41cd..2579ef3984 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
> >  *ret_def = def;
> >  *ret_data = data;
> >  
> > + cleanup:
> >  return fd;
> >  
> >   error:
> >  virDomainDefFree(def);
> >  virQEMUSaveDataFree(data);
> >  VIR_FORCE_CLOSE(fd);
> > -return -1;
> > +goto cleanup;
> 
> As pointed out previously this doesn't really help to make it more
> obvious that 'fd' is abused to cary the other return codes here as well.
> 
> I prefer the following fix:
> 
> https://www.redhat.com/archives/libvir-list/2020-April/msg01101.html

Cool, that's incidentally *precisely* the same patch I'd come up with
initially, before I dug in git history this morning and decided to be
conservative and restore Jiří's original fix instead. :-)

pvl

Re: [PATCH] news: Document new Xen hypervisor features

[Andrea Bolognani]

On Wed, 2020-04-22 at 08:48 -0600, Jim Fehlig wrote:
> On 4/22/20 1:51 AM, Andrea Bolognani wrote:
> > On Tue, 2020-04-21 at 16:06 -0600, Jim Fehlig wrote:
> > > +
> > > +  Xen: add support for e820_host hypervisor feature
> > 
> > s/Xen: add/xen: Add/
> 
> There is only one 'xen:' in news, all other instances are 'Xen:'. I should 
> stick 
> with the majority.

If you look at most (all?) other entries, they look like

  subsystem: Change

which is the same style we use for commit messages.

Xen seems to be the outlier when it comes to following this style,
both in release notes and to some extent in commit messages. It would
be great if it could fall in line with the rest of the code base.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2] qemu: fix domain start with corrupted save file

[Peter Krempa]

On Wed, Apr 22, 2020 at 15:15:31 +0200, Pavel Mores wrote:
> This is to fix
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> 
> With this change, if a domain comes across a corrupted save file during
> boot it removes the save file and logs a warning but continues to boot
> normally instead of failing to boot (with a subsequent boot attempt
> succeeding).
> 
> The regression was introduced by 21ad56e932 and this change effectively
> reverts the relevant part of that commit.
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e8d47a41cd..2579ef3984 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  *ret_def = def;
>  *ret_data = data;
>  
> + cleanup:
>  return fd;
>  
>   error:
>  virDomainDefFree(def);
>  virQEMUSaveDataFree(data);
>  VIR_FORCE_CLOSE(fd);
> -return -1;
> +goto cleanup;

As pointed out previously this doesn't really help to make it more
obvious that 'fd' is abused to cary the other return codes here as well.

I prefer the following fix:

https://www.redhat.com/archives/libvir-list/2020-April/msg01101.html

along with some cleanups which IMO make the function more obvious.
Specifically the non-standard return values. 

[PATCH 4/6] qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded

[Peter Krempa]

Use VIR_AUTOCLOSE to declare it and remove all internal closing of the
filedescriptor. This will allow getting rid of 'error' completely.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 57c66c3401..c0ce1583b1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6691,7 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 bool open_write,
 bool unlink_corrupt)
 {
-int fd = -1;
+VIR_AUTOCLOSE fd = -1;
 int ret = -1;
 g_autoptr(virQEMUSaveData) data = NULL;
 virQEMUSaveHeaderPtr header;
@@ -6723,7 +6723,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 header = &data->header;
 if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) {
 if (unlink_corrupt) {
-if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) {
+if (unlink(path) < 0) {
 virReportSystemError(errno,
  _("cannot remove corrupt file: %s"),
  path);
@@ -6744,7 +6744,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
sizeof(header->magic)) == 0) {
 msg = _("save image is incomplete");
 if (unlink_corrupt) {
-if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) {
+if (unlink(path) < 0) {
 virReportSystemError(errno,
  _("cannot remove corrupt file: %s"),
  path);
@@ -6812,10 +6812,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 *ret_def = g_steal_pointer(&def);
 *ret_data = g_steal_pointer(&data);

-return fd;
+ret = fd;
+fd = -1;
+
+return ret;

  error:
-VIR_FORCE_CLOSE(fd);
 return ret;
 }

-- 
2.26.0

[PATCH 5/6] qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N)

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c0ce1583b1..70cdbe235a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6717,8 +6717,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
VIR_FILE_WRAPPER_BYPASS_CACHE)))
 goto error;

-if (VIR_ALLOC(data) < 0)
-goto error;
+data = g_new0(virQEMUSaveData, 1);

 header = &data->header;
 if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) {
@@ -6783,8 +6782,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,

 cookie_len = header->data_len - xml_len;

-if (VIR_ALLOC_N(data->xml, xml_len) < 0)
-goto error;
+data->xml = g_new0(char, xml_len);

 if (saferead(fd, data->xml, xml_len) != xml_len) {
 virReportError(VIR_ERR_OPERATION_FAILED,
@@ -6793,8 +6791,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 }

 if (cookie_len > 0) {
-if (VIR_ALLOC_N(data->cookie, cookie_len) < 0)
-goto error;
+data->cookie = g_new0(char, cookie_len);

 if (saferead(fd, data->cookie, cookie_len) != cookie_len) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-- 
2.26.0

[PATCH 6/6] qemuDomainSaveImageOpen: Refactor handling of errors

[Peter Krempa]

Return error codes directly and fix weird reporting of errors via
temporary variable.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 51 +-
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 70cdbe235a..00d276061e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6705,17 +6705,18 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 if (directFlag < 0) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("bypass cache unsupported by this system"));
-goto error;
+return -1;
 }
 oflags |= directFlag;
 }

 if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL)) < 0)
-goto error;
+return -1;
+
 if (bypass_cache &&
 !(*wrapperFd = virFileWrapperFdNew(&fd, path,
VIR_FILE_WRAPPER_BYPASS_CACHE)))
-goto error;
+return -1;

 data = g_new0(virQEMUSaveData, 1);

@@ -6726,35 +6727,38 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 virReportSystemError(errno,
  _("cannot remove corrupt file: %s"),
  path);
+return -1;
 } else {
-ret = -3;
+return -3;
 }
-} else {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   "%s", _("failed to read qemu header"));
 }
-goto error;
+
+virReportError(VIR_ERR_OPERATION_FAILED,
+   "%s", _("failed to read qemu header"));
+return -1;
 }

 if (memcmp(header->magic, QEMU_SAVE_MAGIC, sizeof(header->magic)) != 0) {
-const char *msg = _("image magic is incorrect");
-
-if (memcmp(header->magic, QEMU_SAVE_PARTIAL,
-   sizeof(header->magic)) == 0) {
-msg = _("save image is incomplete");
+if (memcmp(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)) == 
0) {
 if (unlink_corrupt) {
 if (unlink(path) < 0) {
 virReportSystemError(errno,
  _("cannot remove corrupt file: %s"),
  path);
+return -1;
 } else {
-ret = -3;
+return -3;
 }
-goto error;
 }
+
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("save image is incomplete"));
+return -1;
 }
-virReportError(VIR_ERR_OPERATION_FAILED, "%s", msg);
-goto error;
+
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("image magic is incorrect"));
+return -1;
 }

 if (header->version > QEMU_SAVE_VERSION) {
@@ -6766,13 +6770,13 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 virReportError(VIR_ERR_OPERATION_FAILED,
_("image version is not supported (%d > %d)"),
header->version, QEMU_SAVE_VERSION);
-goto error;
+return -1;
 }

 if (header->data_len <= 0) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("invalid header data length: %d"), header->data_len);
-goto error;
+return -1;
 }

 if (header->cookieOffset)
@@ -6787,7 +6791,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 if (saferead(fd, data->xml, xml_len) != xml_len) {
 virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("failed to read domain XML"));
-goto error;
+return -1;
 }

 if (cookie_len > 0) {
@@ -6796,7 +6800,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 if (saferead(fd, data->cookie, cookie_len) != cookie_len) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("failed to read cookie"));
-goto error;
+return -1;
 }
 }

@@ -6804,7 +6808,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps,
 VIR_DOMAIN_DEF_PARSE_INACTIVE |
 VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
-goto error;
+return -1;

 *ret_def = g_steal_pointer(&def);
 *ret_data = g_steal_pointer(&data);
@@ -6813,9 +6817,6 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 fd = -1;

 return ret;
-
- error:
-return ret;
 }

 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.26.0

[PATCH 2/6] virQEMUSaveData: Register autoclear function and use it in qemuDomainSaveImageOpen

[Peter Krempa]

In an attempt to simplify qemuDomainSaveImageOpen we need to add
automatic pointer clearing for virQEMUSaveData.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a9361949d..5b87aaf9c2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2856,6 +2856,7 @@ virQEMUSaveDataFree(virQEMUSaveDataPtr data)
 VIR_FREE(data);
 }

+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree);

 /**
  * This function steals @domXML on success.
@@ -6692,7 +6693,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 {
 int fd = -1;
 int ret = -1;
-virQEMUSaveDataPtr data = NULL;
+g_autoptr(virQEMUSaveData) data = NULL;
 virQEMUSaveHeaderPtr header;
 virDomainDefPtr def = NULL;
 int oflags = open_write ? O_RDWR : O_RDONLY;
@@ -6809,13 +6810,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 goto error;

 *ret_def = def;
-*ret_data = data;
+*ret_data = g_steal_pointer(&data);

 return fd;

  error:
 virDomainDefFree(def);
-virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
 return ret;
 }
-- 
2.26.0

[PATCH 0/6] qemuDomainSaveImageOpen: Fix startup with corrupted save image

[Peter Krempa]

This is an alternative to

https://www.redhat.com/archives/libvir-list/2020-April/msg01081.html

which actually fixes qemuDomainSaveImageOpen in a way that would make it
more obvious which value needs to be returned and also cleans up the
code some more.

Peter Krempa (6):
  qemu: fix domain start with corrupted save file
  virQEMUSaveData: Register autoclear function and use it in
qemuDomainSaveImageOpen
  qemuDomainSaveImageOpen: Use g_autoptr for 'dom'
  qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded
  qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N)
  qemuDomainSaveImageOpen: Refactor handling of errors

 src/qemu/qemu_driver.c | 82 +-
 1 file changed, 41 insertions(+), 41 deletions(-)

-- 
2.26.0

[PATCH 1/6] qemu: fix domain start with corrupted save file

[Peter Krempa]

Commit 21ad56e932 introduced a regression where a VM with a corrupted
save image file would fail to start on the first attempt. This was
caused by returning a wrong return code as  'fd' was abused to also hold
the return code.

Since it's easy to miss this nuance introduce a 'ret' variable for the
return code and return it's value in the error section.

https://bugzilla.redhat.com/show_bug.cgi?id=1791522

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dfe0adaad8..9a9361949d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 bool unlink_corrupt)
 {
 int fd = -1;
+int ret = -1;
 virQEMUSaveDataPtr data = NULL;
 virQEMUSaveHeaderPtr header;
 virDomainDefPtr def = NULL;
@@ -6726,7 +6727,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
  _("cannot remove corrupt file: %s"),
  path);
 } else {
-fd = -3;
+ret = -3;
 }
 } else {
 virReportError(VIR_ERR_OPERATION_FAILED,
@@ -6747,7 +6748,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
  _("cannot remove corrupt file: %s"),
  path);
 } else {
-fd = -3;
+ret = -3;
 }
 goto error;
 }
@@ -6816,7 +6817,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 virDomainDefFree(def);
 virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
-return -1;
+return ret;
 }

 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.26.0

[PATCH 3/6] qemuDomainSaveImageOpen: Use g_autoptr for 'dom'

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5b87aaf9c2..57c66c3401 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6695,7 +6695,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 int ret = -1;
 g_autoptr(virQEMUSaveData) data = NULL;
 virQEMUSaveHeaderPtr header;
-virDomainDefPtr def = NULL;
+g_autoptr(virDomainDef) def = NULL;
 int oflags = open_write ? O_RDWR : O_RDONLY;
 size_t xml_len;
 size_t cookie_len;
@@ -6809,13 +6809,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
 goto error;

-*ret_def = def;
+*ret_def = g_steal_pointer(&def);
 *ret_data = g_steal_pointer(&data);

 return fd;

  error:
-virDomainDefFree(def);
 VIR_FORCE_CLOSE(fd);
 return ret;
 }
-- 
2.26.0

Re: [PATCH] news: Document new Xen hypervisor features

[Jim Fehlig]

On 4/22/20 1:51 AM, Andrea Bolognani wrote:

On Tue, 2020-04-21 at 16:06 -0600, Jim Fehlig wrote:

+
+  Xen: add support for e820_host hypervisor feature


s/Xen: add/xen: Add/


There is only one 'xen:' in news, all other instances are 'Xen:'. I should stick 
with the majority.



I suggest also putting either single or double quotes around the name
of the feature for clarity.


+
+
+  e820_host is a Xen-specific option only available for PV guests.


And here you could wrap it in a  element so that it will
display nicely on the Web.

These comments apply also to the second item.


Thanks! I'll address the comments before pushing.

Regards,
Jim

[all PATCH] gitlab: add CI job for validating DCO signoff

[Daniel P . Berrangé]

This job uses the shared "check-dco" image to validate that all
commits on a developer's branch have a suitable Signed-off-by
statement present.

Signed-off-by: Daniel P. Berrangé 
---

This patch was against the Perl repo, but if this is approved then
I'll also apply it to *all* the other repos which currently lack a
DCO check, without reposting further patches for each repo.

For libvirt.git I'll send a patch to update its existing DCO job.

 .gitlab-ci.yml | 16 
 1 file changed, 16 insertions(+)
 create mode 100644 .gitlab-ci.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 000..56e0e02
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,16 @@
+
+stages:
+  - prebuild
+
+# Check that all commits are signed-off for the DCO. Skip
+# on master branch and -maint branches, since we only need
+# to test developer's personal branches.
+check-dco:
+  stage: prebuild
+  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
+  script:
+- /check-dco
+  only:
+- branches
+  except:
+- master
-- 
2.25.2

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Peter Krempa]

On Thu, Apr 16, 2020 at 17:07:35 +0200, Pavel Mores wrote:
> On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote:
> > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > > > +}
> > > > +
> > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > > > +topPath = disk->src->path;
> > > > +else
> > > > +topPath = snapdisk->src->path;
> > > 
> > > You must not use paths for doing this lookup. Paths at best work for
> > > local files only and this would make the code not future proof.
> > > 
> > > Also you want to verify that:
> > > - images you want to merge are actually in the backing chain
> > > - the backing chain looks as snapshots describe it (e.g you unplug vda
> > >   and plug a different chain back)
> > 
> > Let me check with you how exactly to perform this verification.
> > 
> > To retrieve the names of the disks included in a snapshot, I can use its
> > virDomainSnapshotDef which contains an array of disks
> > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify 
> > disks.
> > 
> > To get the state of the chain at moment the snapshot was created, I can use 
> > the
> > 'src' member and walk its 'backingStore' list to examine the whole chain.
> > 
> > To get the currently running configuration, I assume I can use the names of 
> > the
> > relevant disks (obtained from the snapshot as mentioned above) to get a
> > virDomainDiskDefPtr for each of them, whose 'src' member points to a
> > virStorageSource that represents the topmost image in the disk's chain.  
> > From
> > there, I can walk the 'backingStore' list to get the other images in the 
> > chain
> > all the way to the base image.
> > 
> > The verification succeeds if the disk names and their chains in the snapshot
> > and the running config correspond.  Is the above correct?
> > 
> > If so, I'm still not certain how to compare two virStorageSource's.  How is
> > identity of two different virStorageSource instances is established?
> 
> After having a closer look into this I'm unfortunately even less clear as to
> how to perform the checks mentioned in the review.

Well unfortunately developing this is the main part why deleting
snapshots is complicated.

> In addition to the problem of establishing virStorageSource identity, a 
> similar
> problem seems to come up with disks.  They often seem to be identified and
> referred to by the target name, however what happens if a snapshot is taken 
> for
> 'vda', then the disk's target is changed to 'vdb' and now the snapshot is to 
> be

In such case I'd consier it as being different. Similarly as we can't
really guarantee that the image described by a virStorageSource is the
same as it was when taking the snapshot. As long as the file is named
the same you can consider it identical IMO.

> deleted?  Is there a way to find out that the disk is still there, only under 
> a
> different name?

No and it doesn't seem to be worth doing that.

> 
> And as far as comparing chains goes - I know can retrieve the chain for a
> running disk and I can retrieve what the chain looked like at the point a
> snapshot was made.  However, how do I establish they are equivalent?  How can 
> I

What I meant is that you need to establish that the images you are going
to merge and the images you are going to merge into are the same
described by the snapshots and at the same time present in the current
backing chain of the disk. If they are not in the snapshot metadata
you'd potentially merge something unwanted, but this is covered by the
previous paragraphs.

You need to also verify that they are currently opened by qemu as you
can't do blockjobs on images which are not part of the chain.

> tell that snapshots in both chains compare identical in the first place?  Is
> the snapshot name stable and persistent enough to be useful for this?  Is it
> enough for chains to be equivalent that the chain at the point of snapshot
> creation be a superset of the currently running chain?  Probably yes, provided
> snapshots can't be inserted in the middle of a chain - but could that ever
> happen?

The main problem is that the configuration may change from the time when
the snapshot was taken. Either the XML or the on disk setup. In case a
user issues a blockjob manually which merges parts of the chain you
won't be able to delete the snapshot with data reorganisation via the
API. Similarly to when the VM config changes to point to other images
and or remove or add disks. The snapshot code should be able to at least
properly reject the operation if it's unclear whether we can do the
right thing.

Unfortunately all the above need to be considered and thought out before
because there isn't really any prior art in libvirt that could be
copied. There are helpers to compare two virStorageSource structs, but
that's far from all of the required logic.

While the initial 

Re: [PATCH v2 1/2] conf: Add option for settings

[Daniel Henrique Barboza]

On 4/22/20 9:22 AM, Julio Faracco wrote:

Hi Daniel,

Thanks for reviewing. :-)



Np, glad to be of assistance



IMHO, I don't like to join them in one single patch because it is hard
to review.



Hmm, reading this I believe that the tests would pass after applying patch
2/2 then (didn't try).


I know that others have a different opinion (and it is good).



The way you split the patch series is up to debate and so on and so
forth. The issue here is that every single patch must pass 'make check',
as said in docs/hacking.rst in the "Preparing patches" session:

--
If you're going to submit multiple patches, the automated tests
must pass **after each patch**, not just after the last one.
--

You'll need to think in a way of splitting the series that allows each
patch to pass 'make check' in its own.


Thanks,


DHB

[libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file

[Erik Skultety]

Rather than having the configuration options split across multiple files
(root-password, flavor, gitlab-url, gitlab-runner-token, ...), let's
consolidate these settings into a global config file.

The TOML format has been chosen simply because its syntax is very similar
to the conventional INI format (thus easily human readable),
but unlike INI it has a formal specification, it's typed and it also
has some nice features over the plain INI should we find ourselves in
need of any of such extended features in the future, e.g. table nesting.

Signed-off-by: Erik Skultety 
---
 config.toml | 26 ++
 1 file changed, 26 insertions(+)
 create mode 100644 config.toml

diff --git a/config.toml b/config.toml
new file mode 100644
index 000..5bb3725
--- /dev/null
+++ b/config.toml
@@ -0,0 +1,26 @@
+# Configuration file for the lcitool -- https://gitlab.com/libvirt/libvirt-ci/
+# 
+
+[install]
+# Installation flavor determining the target environment for the VM:
+#
+# test - VMs suitable for local testing, 'test' has passwordless sudo
+# jenkins - VMs pluggable to a jenkins environment
+# gitlab - VMs ready to be plugged to a GitLab environment
+#
+#flavor = "test"
+
+# Initial root password to be set by ansible on the appliance. This password
+# will only be necessary for serial console access in case something goes
+# horribly wrong, for all other use cases, SSH key authentication will be used
+# instead. (mandatory)
+#root_password = ""
+
+[gitlab]
+# GitLab runner agent registration options, applies only if flavor == 'gitlab'.
+
+# GitLab server URL to register the runner.
+#gitlab_url = "https://gitlab.com";
+
+# GitLab runner registration token. (mandatory)
+#gitlab_runner_secret = ""
-- 
2.25.3

[libvirt-ci PATCH 08/13] lcitool: Drop the get_root_password_file() method

[Erik Skultety]

We can now access this value directly in the config dictionary.

Signed-off-by: Erik Skultety 
---
 guests/lcitool  | 19 +--
 guests/playbooks/update/tasks/users.yml |  2 +-
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index a385c15..6c905c2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -227,22 +227,6 @@ class Config:
 
 return vault_pass_file
 
-def get_root_password_file(self):
-root_pass_file = self._get_config_file("root-password")
-
-try:
-with open(root_pass_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid root password file ({}): {}".format(
-root_pass_file, ex
-)
-)
-
-return root_pass_file
-
 def get_gitlab_runner_token_file(self):
 if self.get_flavor() != "gitlab":
 return None
@@ -511,7 +495,6 @@ class Application:
 base = Util.get_base()
 
 vault_pass_file = self._config.get_vault_password_file()
-root_pass_file = self._config.get_root_password_file()
 gitlab_url_file = self._config.get_gitlab_url_file()
 gitlab_runner_token_file = self._config.get_gitlab_runner_token_file()
 
@@ -541,7 +524,7 @@ class Application:
 extra_vars = {
 "base": base,
 "playbook_base": playbook_base,
-"root_password_file": root_pass_file,
+"root_password": self._config.dict["install"]["root_password"],
 "flavor": self._config.dict["install"]["flavor"],
 "selected_projects": selected_projects,
 "git_remote": git_remote,
diff --git a/guests/playbooks/update/tasks/users.yml 
b/guests/playbooks/update/tasks/users.yml
index 28ee96a..8744290 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -2,7 +2,7 @@
 - name: 'root: Set password'
   user:
 name: root
-password: '{{ lookup("file", root_password_file)|password_hash("sha512") 
}}'
+password: '{{ root_password|password_hash("sha512") }}'
 
 - name: 'root: Configure ssh access'
   authorized_key:
-- 
2.25.3

[libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method

[Erik Skultety]

We can now access this value directly in the config dictionary.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 32 +++-
 1 file changed, 3 insertions(+), 29 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index f08bd98..a385c15 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -206,37 +206,12 @@ class Config:
 if flavor == "gitlab":
 self._validate_section(self.dict, "gitlab", "gitlab_runner_secret")
 
-def get_flavor(self):
-flavor_file = self._get_config_file("flavor")
-
-try:
-with open(flavor_file, "r") as infile:
-flavor = infile.readline().strip()
-except Exception:
-# If the flavor has not been configured, we choose the default
-# and store it on disk to ensure consistent behavior from now on
-flavor = "test"
-try:
-with open(flavor_file, "w") as infile:
-infile.write("{}\n".format(flavor))
-except Exception as ex:
-raise Exception(
-"Can't write flavor file ({}): {}".format(
-flavor_file, ex
-)
-)
-
-if flavor not in ["test", "jenkins", "gitlab"]:
-raise Exception("Invalid flavor '{}'".format(flavor))
-
-return flavor
-
 def get_vault_password_file(self):
 vault_pass_file = None
 
 # The vault password is only needed for the jenkins flavor, but in
 # that case we want to make sure there's *something* in there
-if self.get_flavor() == "jenkins":
+if self.dict["install"]["flavor"] == "jenkins":
 vault_pass_file = self._get_config_file("vault-password")
 
 try:
@@ -535,7 +510,6 @@ class Application:
 def _execute_playbook(self, playbook, hosts, projects, git_revision):
 base = Util.get_base()
 
-flavor = self._config.get_flavor()
 vault_pass_file = self._config.get_vault_password_file()
 root_pass_file = self._config.get_root_password_file()
 gitlab_url_file = self._config.get_gitlab_url_file()
@@ -568,7 +542,7 @@ class Application:
 "base": base,
 "playbook_base": playbook_base,
 "root_password_file": root_pass_file,
-"flavor": flavor,
+"flavor": self._config.dict["install"]["flavor"],
 "selected_projects": selected_projects,
 "git_remote": git_remote,
 "git_branch": git_branch,
@@ -617,7 +591,7 @@ class Application:
 def _action_install(self, args):
 base = Util.get_base()
 
-flavor = self._config.get_flavor()
+flavor = self._config.dict["install"]["flavor"]
 
 for host in self._inventory.expand_pattern(args.hosts):
 facts = self._inventory.get_facts(host)
-- 
2.25.3

[libvirt-ci PATCH 09/13] lcitool: Drop the gitlab-related getter methods

[Erik Skultety]

We can now access the values directly in the config dictionary.

Signed-off-by: Erik Skultety 
---
 guests/lcitool   | 44 ++--
 guests/playbooks/update/tasks/gitlab.yml |  2 --
 2 files changed, 2 insertions(+), 44 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 6c905c2..f2b4d44 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -227,44 +227,6 @@ class Config:
 
 return vault_pass_file
 
-def get_gitlab_runner_token_file(self):
-if self.get_flavor() != "gitlab":
-return None
-
-gitlab_runner_token_file = self._get_config_file("gitlab-runner-token")
-
-try:
-with open(gitlab_runner_token_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid GitLab runner token file ({}): {}".format(
-gitlab_runner_token_file, ex
-)
-)
-
-return gitlab_runner_token_file
-
-def get_gitlab_url_file(self):
-if self.get_flavor() != "gitlab":
-return None
-
-gitlab_url_file = self._get_config_file("gitlab-url")
-
-try:
-with open(gitlab_url_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid GitLab url file ({}): {}".format(
-gitlab_url_file, ex
-)
-)
-
-return gitlab_url_file
-
 
 class Inventory:
 
@@ -495,8 +457,6 @@ class Application:
 base = Util.get_base()
 
 vault_pass_file = self._config.get_vault_password_file()
-gitlab_url_file = self._config.get_gitlab_url_file()
-gitlab_runner_token_file = self._config.get_gitlab_runner_token_file()
 
 ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
 selected_projects = self._projects.expand_pattern(projects)
@@ -529,8 +489,8 @@ class Application:
 "selected_projects": selected_projects,
 "git_remote": git_remote,
 "git_branch": git_branch,
-"gitlab_url_file": gitlab_url_file,
-"gitlab_runner_token_file": gitlab_runner_token_file,
+"gitlab_url": self._config.dict["gitlab"]["url"],
+"gitlab_runner_secret": self._config.dict["gitlab"]["token"],
 }
 json.dump(extra_vars, fp)
 
diff --git a/guests/playbooks/update/tasks/gitlab.yml 
b/guests/playbooks/update/tasks/gitlab.yml
index 7691442..c13ca22 100644
--- a/guests/playbooks/update/tasks/gitlab.yml
+++ b/guests/playbooks/update/tasks/gitlab.yml
@@ -1,8 +1,6 @@
 ---
 - name: Define gitlab-related facts
   set_fact:
-gitlab_url: '{{ lookup("file", gitlab_url_file) }}'
-gitlab_runner_secret: '{{ lookup("file", gitlab_runner_token_file) }}'
 gitlab_runner_download_url: 
https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-{{
 ansible_system|lower }}-amd64
 gitlab_runner_config_dir: '/etc/gitlab-runner'
 
-- 
2.25.3

[libvirt-ci PATCH 03/13] lcitool: Prefer tempfile's native wrappers over low level primitives

[Erik Skultety]

Rather than requiring shutil module to get rid of the temporary
directory we're creating for virt-install, let's use the
TemporaryDirectory method instead which returns a file-like object which
can be used to clean up the standard Python way.
Although the internal exit handlers will take care of closing the
temporary directories (and thus removing their contents) automatically,
let's be explicit anyway and use the 'finally' clause to clean these up
on both success and failure.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 9a9326d..51ee211 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -23,7 +23,6 @@ import json
 import os
 import platform
 import random
-import shutil
 import string
 import subprocess
 import sys
@@ -608,8 +607,8 @@ class Application:
 facts[option]
 )
 
-tempdir = tempfile.mkdtemp()
-initrd_inject = os.path.join(tempdir, install_config)
+tempdir = tempfile.TemporaryDirectory()
+initrd_inject = os.path.join(tempdir.name, install_config)
 
 with open(initrd_inject, "w") as inject:
 inject.write(content)
@@ -663,8 +662,8 @@ class Application:
 subprocess.check_call(cmd)
 except Exception as ex:
 raise Exception("Failed to install '{}': {}".format(host, ex))
-
-shutil.rmtree(tempdir, ignore_errors=True)
+finally:
+tempdir.cleanup()
 
 def _action_update(self, args):
 self._execute_playbook("update", args.hosts, args.projects,
-- 
2.25.3

[libvirt-ci PATCH 02/13] lcitool: Decrease the indent when creating a tempdir for initrd injection

[Erik Skultety]

The 'with' statement doesn't define a new code block [1], thus no local
namespace is created. Therefore, we can still access the @content
variable outside of the 'with' block. So, there's really no need to
hold the @initrd_template file open longer than necessary (not that it
would be a big deal anyway).

[1] https://docs.python.org/3.7/reference/executionmodel.html

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index dfb1ebc..9a9326d 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -608,11 +608,11 @@ class Application:
 facts[option]
 )
 
-tempdir = tempfile.mkdtemp()
-initrd_inject = os.path.join(tempdir, install_config)
+tempdir = tempfile.mkdtemp()
+initrd_inject = os.path.join(tempdir, install_config)
 
-with open(initrd_inject, "w") as inject:
-inject.write(content)
+with open(initrd_inject, "w") as inject:
+inject.write(content)
 
 # preseed files must use a well-known name to be picked up by
 # d-i; for kickstart files, we can use whatever name we please
-- 
2.25.3

[libvirt-ci PATCH 13/13] guests: README: Document the existence and usage of config.toml

[Erik Skultety]

Signed-off-by: Erik Skultety 
---
 guests/README.markdown | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/guests/README.markdown b/guests/README.markdown
index 7dbefed..d9afbd3 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -64,13 +64,16 @@ branches out of those with
 Host setup
 --
 
-Ansible and `virt-install` need to be available on the host.
+`ansible`, `python3-toml` and `virt-install` need to be available on the host,
+the former two can be either installed system-wide using your package manager
+or using by `pip` (see the provided requirements.txt file). The latter can only
+be installed with your package manager as `virt-install` is not distributed via
+PyPi.
 
-Before you can start bringing up guests, you'll have to store your
-site-specific root password in the `~/.config/lcitool/root-password` file.
-This password will only be necessary for serial console access in case
-something goes horribly wrong; for day to day operations, SSH key
-authentication will be used instead.
+Before you can start bringing up guests, you need to create (ideally by
+copying the `config.toml` template) ~/.config/lcitool/config.toml and set at
+least the options marked as "(mandatory)" depending on the flavor (`test`,
+`jenkins`, `gitlab`) you wish to use with your machines.
 
 Ansible expects to be able to connect to the guests by name: installing and
 enabling the [libvirt NSS plugin](https://wiki.libvirt.org/page/NSS_module)
@@ -123,8 +126,8 @@ Jenkins CI use
 --
 
 You'll need to configure `lcitool` to use the `jenkins` flavor for
-guests: to do so, just write `jenkins` in the `~/.config/lcitool/flavor`
-file.
+guests. To do so, simply set the flavor to 'jenkins' in
+`~/.config/lcitool/config.toml`.
 
 Once a guest has been prepared, you'll be able to log in as root either
 via SSH (your public key will have been authorized) or on the serial
-- 
2.25.3

[libvirt-ci PATCH 10/13] lcitool: Use d.update() on extra_vars for options coming from the config

[Erik Skultety]

Rather than putting all the options to the extra_vars JSON, use the
update method to extend the source dictionary with options coming from
the lcitool config file. By doing this split, we know which options are
hard-coded and which come from external sources. The main reason for this
change though is that some sections/members in the config file are
optional (thus may be missing in the config dictionary) and we'd risk
the KeyError exception if we tried to access them directly when filling
out the extra_vars JSON.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index f2b4d44..4cb6e69 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -481,17 +481,22 @@ class Application:
 extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')
 
 with open(extra_vars_path, 'w') as fp:
+
+# start with generic items not coming from the config
 extra_vars = {
 "base": base,
 "playbook_base": playbook_base,
-"root_password": self._config.dict["install"]["root_password"],
-"flavor": self._config.dict["install"]["flavor"],
 "selected_projects": selected_projects,
 "git_remote": git_remote,
 "git_branch": git_branch,
-"gitlab_url": self._config.dict["gitlab"]["url"],
-"gitlab_runner_secret": self._config.dict["gitlab"]["token"],
 }
+
+# now add the config vars
+extra_vars.update(self._config.dict["install"])
+
+if extra_vars["flavor"] == "gitlab":
+extra_vars.update(self._config.dict["gitlab"])
+
 json.dump(extra_vars, fp)
 
 ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
-- 
2.25.3

[libvirt-ci PATCH 12/13] lcitool: Use the config install options rather than install facts

[Erik Skultety]

Now that we have the corresponding options available in the config,
let's start using them.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 30 +++---
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 36320ab..9a9256f 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -565,25 +565,17 @@ class Application:
 
 def _action_install(self, args):
 base = Util.get_base()
-
-flavor = self._config.dict["install"]["flavor"]
+cfg = self._config.dict["install"]
 
 for host in self._inventory.expand_pattern(args.hosts):
 facts = self._inventory.get_facts(host)
 
-# Both memory size and disk size are stored as GiB in the
-# inventory, but virt-install expects the disk size in GiB
-# and the memory size in *MiB*, so perform conversion here
-memory_arg = str(int(facts["install_memory_size"]) * 1024)
-
-vcpus_arg = str(facts["install_vcpus"])
-
 disk_arg = "size={},pool={},bus=virtio".format(
-facts["install_disk_size"],
-facts["install_storage_pool"],
+cfg["disk_size"],
+cfg["storage_pool"],
 )
 network_arg = "network={},model=virtio".format(
-facts["install_network"],
+cfg["network"],
 )
 
 # Different operating systems require different configuration
@@ -643,12 +635,12 @@ class Application:
 virt_install,
 "--name", host,
 "--location", facts["install_url"],
-"--virt-type", facts["install_virt_type"],
-"--arch", facts["install_arch"],
-"--machine", facts["install_machine"],
-"--cpu", facts["install_cpu_model"],
-"--vcpus", vcpus_arg,
-"--memory", memory_arg,
+"--virt-type", cfg["virt_type"],
+"--arch", cfg["arch"],
+"--machine", cfg["machine"],
+"--cpu", cfg["cpu_model"],
+"--vcpus", str(cfg["vcpus"]),
+"--memory", str(cfg["memory_size"] * 1024),
 "--disk", disk_arg,
 "--network", network_arg,
 "--graphics", "none",
@@ -663,7 +655,7 @@ class Application:
 cmd.append("--noautoconsole")
 
 # Only configure autostart for the guest for the jenkins flavor
-if flavor == "jenkins":
+if cfg["flavor"] == "jenkins":
 cmd += ["--autostart"]
 
 try:
-- 
2.25.3

[libvirt-ci PATCH 11/13] config: Move the virt-install settings from install.yml to the config

[Erik Skultety]

Looking into the future where we're able to generate cloudinit images,
we'll need to configure some of the install options which is currently
not possible without editing the install.yml group vars file within the
repository. That is suboptimal, so let's move the install options to
the global config under the '[install]' section so that further tweaking
is possible (but explicitly discouraged at the same time).

Signed-off-by: Erik Skultety 
---
 config.toml| 17 +
 guests/lcitool | 27 +++
 2 files changed, 44 insertions(+)

diff --git a/config.toml b/config.toml
index 5bb3725..9bb1b7f 100644
--- a/config.toml
+++ b/config.toml
@@ -16,6 +16,23 @@
 # instead. (mandatory)
 #root_password = ""
 
+# Settings mapping to the virt-install options - see virt-install(1).
+# It is strongly recommended that you keep the following at their default
+# values to produce machines which conform to the upstream libvirt standard,
+# unless you have a reason to do otherwise.
+#
+# Sizes are expressed in GiB.
+#
+#virt_type = "kvm"
+#arch = "x86_64"
+#machine = "pc"
+#cpu_model = "host-passthrough"
+#vcpus = 2
+#memory_size = 2
+#disk_size = 15
+#storage_pool = "default"
+#network = "default"
+
 [gitlab]
 # GitLab runner agent registration options, applies only if flavor == 'gitlab'.
 
diff --git a/guests/lcitool b/guests/lcitool
index 4cb6e69..36320ab 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -175,6 +175,33 @@ class Config:
 flavor = cfg.get("install").get("flavor", "test")
 cfg["install"]["flavor"] = flavor
 
+virt_type = cfg.get("install").get("virt_type", "kvm")
+cfg["install"]["virt_type"] = virt_type
+
+arch = cfg.get("install").get("arch", "x86_64")
+cfg["install"]["arch"] = arch
+
+machine = cfg.get("install").get("machine", "pc")
+cfg["install"]["machine"] = machine
+
+cpu_model = cfg.get("install").get("cpu_model", "host-passthrough")
+cfg["install"]["cpu_model"] = cpu_model
+
+vcpus = cfg.get("install").get("vcpus", 2)
+cfg["install"]["vcpus"] = vcpus
+
+memory_size = cfg.get("install").get("memory_size", 2)
+cfg["install"]["memory_size"] = memory_size
+
+disk_size = cfg.get("install").get("disk_size", 15)
+cfg["install"]["disk_size"] = disk_size
+
+storage_pool = cfg.get("install").get("storage_pool", "default")
+cfg["install"]["storage_pool"] = storage_pool
+
+network = cfg.get("install").get("network", "default")
+cfg["install"]["network"] = network
+
 if flavor == "gitlab":
 url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com";)
 cfg["gitlab"]["gitlab_url"] = url
-- 
2.25.3

[libvirt-ci RFC PATCH 00/13] Introduce a new global TOML config file for lcitool

[Erik Skultety]

This series is trying to consolidate the number of config files we currently
recognize under ~/.config/lcitool to a single global TOML config file. Thanks
to this effort we can expose more seetings than we previously could which will
come handy in terms of generating cloudinit images for OpenStack.

Patches 1-4 patches are just a little extra - not heavily related to the series
See patch 5/13 why TOML was selected.

Erik Skultety (13):
  requirements: Introduce a requirements.txt file
  lcitool: Decrease the indent when creating a tempdir for initrd
injection
  lcitool: Prefer tempfile's native wrappers over low level primitives
  lcitool: Use a temporary JSON file to pass extra variables
  config: Introduce a new global config.toml configuration file
  lcitool: Introduce methods to load and validate the TOML config
  lcitool: Drop the get_flavor() method
  lcitool: Drop the get_root_password_file() method
  lcitool: Drop the gitlab-related getter methods
  lcitool: Use d.update() on extra_vars for options coming from the
config
  config: Move the virt-install settings from install.yml to the config
  lcitool: Use the config install options rather than install facts
  guests: README: Document the existence and usage of config.toml

 config.toml  |  43 +
 guests/README.markdown   |  19 +-
 guests/lcitool   | 235 +++
 guests/playbooks/update/tasks/gitlab.yml |   2 -
 guests/playbooks/update/tasks/users.yml  |   2 +-
 requirements.txt |   3 +
 6 files changed, 175 insertions(+), 129 deletions(-)
 create mode 100644 config.toml
 create mode 100644 requirements.txt

-- 
2.25.3

[libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config

[Erik Skultety]

This patch introduce a set of class Config helper methods in order to
parse and validate the new global TOML config.
Currently, only 'install' and 'gitlab' sections are recognized with
the flavor setting defaulting to "test" (backwards compatibility) and
gitlab runner registration url defaulting to "https://gitlab.com";; the
rest of the options are currently mandatory.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 52 ++
 1 file changed, 52 insertions(+)

diff --git a/guests/lcitool b/guests/lcitool
index 138b5e2..f08bd98 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -28,6 +28,7 @@ import subprocess
 import sys
 import tempfile
 import textwrap
+import toml
 import yaml
 
 # This is necessary to maintain Python 2.7 compatibility
@@ -133,6 +134,21 @@ class Util:
 
 class Config:
 
+def __init__(self):
+path = self._get_config_file("config.toml")
+
+try:
+self.dict = toml.load(path)
+self._validate()
+
+except Exception as ex:
+raise Exception(
+"Missing or invalid config.toml file: {}".format(ex)
+)
+
+# fill in default options
+self._fill_default_options(self.dict)
+
 @staticmethod
 def _get_config_file(name):
 try:
@@ -154,6 +170,42 @@ class Config:
 
 return os.path.join(config_dir, name)
 
+@staticmethod
+def _fill_default_options(cfg):
+flavor = cfg.get("install").get("flavor", "test")
+cfg["install"]["flavor"] = flavor
+
+if flavor == "gitlab":
+url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com";)
+cfg["gitlab"]["gitlab_url"] = url
+
+@staticmethod
+def _validate_section(cfg, section, *args):
+
+if cfg.get(section, None) is None:
+raise KeyError("Section '[{}]' not found".format(section))
+
+for key in args:
+if cfg.get(section).get(key, None) is None:
+raise KeyError("Missing mandatory key '{}.{}'".format(section,
+  key))
+
+def _validate(self):
+
+# verify the [install] section and its mandatory options
+self._validate_section(self.dict, "install", "root_password")
+
+# we need to check flavor here, because later validations depend on it
+flavor = self.dict.get("install").get("flavor", "test")
+if flavor not in ["test", "jenkins", "gitlab"]:
+raise ValueError(
+"Invalid value '{}' for 'install.flavor'".format(flavor)
+)
+
+# verify the optional [gitlab] section and its mandatory options
+if flavor == "gitlab":
+self._validate_section(self.dict, "gitlab", "gitlab_runner_secret")
+
 def get_flavor(self):
 flavor_file = self._get_config_file("flavor")
 
-- 
2.25.3

[libvirt-ci PATCH 01/13] requirements: Introduce a requirements.txt file

[Erik Skultety]

Right now, lcitool's users have 2 options to satisfy the tool's
dependencies: install them through the system package manager
system-wide or install through pip manually.
Since pip gives us the option to automate this process a tiny bit, let's
ship a requirements file. This targets users who are used to work
with Python virtual environments and install whatever it is necessary
inside.

Signed-off-by: Erik Skultety 
---
 requirements.txt | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 requirements.txt

diff --git a/requirements.txt b/requirements.txt
new file mode 100644
index 000..dc416fa
--- /dev/null
+++ b/requirements.txt
@@ -0,0 +1,3 @@
+ansible
+jenkins-job-builder
+toml
-- 
2.25.3

[libvirt-ci PATCH 04/13] lcitool: Use a temporary JSON file to pass extra variables

[Erik Skultety]

This patch is a pre-requisite config file consolidation. Currently we've
got a number of files which serve as a configuration either to the
lcitool itself or to the ansible playbooks (majority).  Once we replace
these with a single global lcitool config, we'd end up passing tokens
(potentially some passwords) as ansible extra variables bare naked on
the cmdline. In order to prevent this security flaw use temporary JSON
file holding all these extra variables and pass it as follows:

$ ansible-playbook --extra-vars @extra_vars.json playbook.yml

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 31 +++
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 51ee211..138b5e2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -504,21 +504,26 @@ class Application:
 git_remote = "default"
 git_branch = "master"
 
+tempdir = tempfile.TemporaryDirectory(prefix='lcitool')
+
 ansible_cfg_path = os.path.join(base, "ansible.cfg")
 playbook_base = os.path.join(base, "playbooks", playbook)
 playbook_path = os.path.join(playbook_base, "main.yml")
+extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')
 
-extra_vars = json.dumps({
-"base": base,
-"playbook_base": playbook_base,
-"root_password_file": root_pass_file,
-"flavor": flavor,
-"selected_projects": selected_projects,
-"git_remote": git_remote,
-"git_branch": git_branch,
-"gitlab_url_file": gitlab_url_file,
-"gitlab_runner_token_file": gitlab_runner_token_file,
-})
+with open(extra_vars_path, 'w') as fp:
+extra_vars = {
+"base": base,
+"playbook_base": playbook_base,
+"root_password_file": root_pass_file,
+"flavor": flavor,
+"selected_projects": selected_projects,
+"git_remote": git_remote,
+"git_branch": git_branch,
+"gitlab_url_file": gitlab_url_file,
+"gitlab_runner_token_file": gitlab_runner_token_file,
+}
+json.dump(extra_vars, fp)
 
 ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
 if ansible_playbook is None:
@@ -527,7 +532,7 @@ class Application:
 cmd = [
 ansible_playbook,
 "--limit", ansible_hosts,
-"--extra-vars", extra_vars,
+"--extra-vars", "@" + extra_vars_path,
 ]
 
 # Provide the vault password if available
@@ -546,6 +551,8 @@ class Application:
 except Exception as ex:
 raise Exception(
 "Failed to run {} on '{}': {}".format(playbook, hosts, ex))
+finally:
+tempdir.cleanup()
 
 def _action_hosts(self, args):
 for host in self._inventory.expand_pattern("all"):
-- 
2.25.3

[libvirt PATCH v2] qemu: fix domain start with corrupted save file

[Pavel Mores]

This is to fix

https://bugzilla.redhat.com/show_bug.cgi?id=1791522

With this change, if a domain comes across a corrupted save file during
boot it removes the save file and logs a warning but continues to boot
normally instead of failing to boot (with a subsequent boot attempt
succeeding).

The regression was introduced by 21ad56e932 and this change effectively
reverts the relevant part of that commit.
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8d47a41cd..2579ef3984 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 *ret_def = def;
 *ret_data = data;
 
+ cleanup:
 return fd;
 
  error:
 virDomainDefFree(def);
 virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
-return -1;
+goto cleanup;
 }
 
 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.24.1

Re: [libvirt PATCH] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 01:07:30PM +0200, Peter Krempa wrote:
> On Wed, Apr 22, 2020 at 12:54:26 +0200, Pavel Mores wrote:
> > This is to fix
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> 
> Please describe the bug in the commit message rather than have users
> open and try understand external links.

It's described in the next paragraph.

> 
> 
> > 
> > With this change, if a domain comes across a corrupted save file during 
> > boot it
> > removes the save file and logs a warning but continues to boot normally 
> > instead
> > of failing to boot (with a subsequent boot attempt succeeding).
> > 
> > The regression was introduced by 21ad56e932 and this change effectively 
> > reverts
> > the relevant part of that commit.
> 
> Please break commit messages at 72 colums as it's common.
> 
> 
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index e8d47a41cd..2579ef3984 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
> >  *ret_def = def;
> >  *ret_data = data;
> >  
> > + cleanup:
> >  return fd;
> >  
> >   error:
> >  virDomainDefFree(def);
> >  virQEMUSaveDataFree(data);
> >  VIR_FORCE_CLOSE(fd);
> > -return -1;
> > +goto cleanup;
> 
> I think this should be fixed properly. 'fd' is serving double duty of
> also reporting the erorrs here and it's not really obvious from this
> hunk. The code above sets 'fd' to a variety of values e.g. -3 on some
> errors. This is really tricky and hard to follow.
> 
> In this case we want a 'ret' variable which is set to the proper value
> and returned on error and fd returned on success or alternatively
> assigned to 'ret' right before returning it. This should make it
> possible to do it even without adding new not really useful labels.
> 

Re: Re: [RFC 00/29] RFC: Generate object-model code based on relax-ng files

[Andrea Bolognani]

On Wed, 2020-04-22 at 10:45 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 11:37:23AM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-04-22 at 09:48 +0100, Daniel P. Berrangé wrote:
> > > I wonder if we can refactor that tool to extract the code for parsing into
> > > a module, so that we can more reasily re-use it for both the API docs and
> > > for a new XML generator
> > 
> > Or we could replace that C parsing code with something based on
> > libclang.
> 
> I think that's a double edged sword. While it gives you the full coverage
> of the C language, you then have to deal with the full range of the C
> language. I think the simplified parser we have for the docs build will
> be easier to work with by being more constrained in what it tries to
> support.

Glancing at the ~2000 lines of Python used to parse C doesn't
necessarily give me the same impression :) But then again, in all
fairness I have no idea how complicated an equivalent tool that uses
libclang would be.

> > Either way, if this ever becomes usable I think it should not live
> > in the libvirt repository but be a standalone tool instead, as I can
> > see many projects potentially benefiting from it.
> 
> Yes, but we can worry about that at a later date IMHO.

Absolutely! Let's just keep this goal in mind, and try to make it
reasonably generic instead of hardcoding libvirt-concepts in it.

> > Which begs the question: are we absolutely certain something like
> > this doesn't exist already? We should make sure that's really the
> > case before we invest time on it...
> 
> I've not found anything equivalent to Golang's XML parser for C

Hopefully the tool we're thinking of is not just hidden in some
random person's GitHub account :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/2] qemu: Add some cpu hotpluggable inforamtion in vm xml when run "virsh setvcpus vm --count --live".

[xiajidong]

On 4/9/20 9:23 AM, Peter Krempa wrote:

On Thu, Apr 09, 2020 at 20:38:47 +0800, Jidong Xia wrote:

From: xiajidong 

The optional cpu attribute current can be used to specify whether
fewer than the maximum number of virtual CPUs should be enabled.
when run "virsh setvcpus vm --count  --live", it can
display cpu hotpluggable information when run "virsh dumpxml vm
| grep vcpu". so we can know the value of enabled and hotpluggable
attributes of the current vcpus.

Signed-off-by: Jidong Xia 
---
  src/qemu/qemu_hotplug.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 14654a1..c574f63 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6168,6 +6168,10 @@ qemuDomainSelectHotplugVcpuEntities(virDomainDefPtr def,
  } else {
  *enable = false;

+if (nvcpus == curvcpus) {
+def->individualvcpus = true;
+}


The 'individualvcpus' flag here is deliberately not asserted in this API
so that if only old/legacy vcpu hotplug APIs are used
(qemuDomainSetVcpusFlags, note the plural Vcpus) we don't change the
behaviour or XML.

With new APIs used (qemuDomainSetVcpu, virsh setvcpu - note singular
vcpu) the flag is asserted always and the output now displays
everything.

The commit message plainly states the facts when the extended cpu state
is displayed, but does not justify why you want to change it, so please
add a justification.

Additionally if you justify the change enough, the correct fix will be
to remove individualvcpus completely from 'def' and always display the
information in the XML rather than adding these hacks.I think you are right, I 
am considering and studying this area.

Thanks & Best Regards
Jidong Xia
 

Re: [PATCH v2 1/2] conf: Add option for settings

[Julio Faracco]

Hi Daniel,

Thanks for reviewing. :-)
IMHO, I don't like to join them in one single patch because it is hard
to review.
I know that others have a different opinion (and it is good).
But I will think about it for v3.

--
Julio Cesar Faracco

Em qua., 22 de abr. de 2020 às 08:05, Daniel Henrique Barboza
 escreveu:
>
>
>
> On 4/21/20 1:03 AM, Julio Faracco wrote:
> > If an user is trying to configure a dhcp neetwork settings, it is not
>
> s/neetwork/network
>
>
>
> This patch failed to compile in my box on top of master at 9a13704818e:
>
>
>CCLD libvirdeterministichashmock.la
> ../../tests/networkxml2conftest.c: In function 'testCompareXMLToConfFiles':
> ../../tests/networkxml2conftest.c:46:59: error: passing argument 4 of 
> 'networkDnsmasqConfContents' from incompatible pointer type 
> [-Werror=incompatible-pointer-types]
> 46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, 
> caps) < 0)
>|   ^~~~
>|   |
>|   
> dnsmasqContext * {aka struct  *}
> In file included from ../../tests/networkxml2conftest.c:13:
> ../../src/network/bridge_driver.h:49:35: note: expected 'char **' but 
> argument is of type 'dnsmasqContext *' {aka 'struct  *'}
> 49 |char **hostsfilestr,
>|~~~^~~~
> ../../tests/networkxml2conftest.c:46:65: error: passing argument 5 of 
> 'networkDnsmasqConfContents' from incompatible pointer type 
> [-Werror=incompatible-pointer-types]
> 46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, 
> caps) < 0)
>| ^~~~
>| |
>| 
> dnsmasqCapsPtr {aka struct _dnsmasqCaps *}
> In file included from ../../tests/networkxml2conftest.c:13:
> ../../src/network/bridge_driver.h:50:44: note: expected 'dnsmasqContext *' 
> {aka 'struct  *'} but argument is of type 'dnsmasqCapsPtr' {aka 
> 'struct _dnsmasqCaps *'}
> 50 |dnsmasqContext *dctx,
>|^~~~
> ../../tests/networkxml2conftest.c:46:9: error: too few arguments to function 
> 'networkDnsmasqConfContents'
> 46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, 
> caps) < 0)
>| ^~
> In file included from ../../tests/networkxml2conftest.c:13:
> ../../src/network/bridge_driver.h:46:1: note: declared here
> 46 | networkDnsmasqConfContents(virNetworkObjPtr obj,
>| ^~
> cc1: all warnings being treated as errors
>
>
>
>
> Thanks,
>
>
> DHB

Re: [PATCH] qemucapstest: Refresh data for qemu 5.0 on x86_64

[Ján Tomko]

On a Wednesday in 2020, Peter Krempa wrote:

Bump to v5.0.0-rc3-8-g3119154db0 and make sure that 'liburing' is picked
up by qemu.

Signed-off-by: Peter Krempa 
---
.../caps_5.0.0.x86_64.replies | 99 +--
.../caps_5.0.0.x86_64.xml | 68 -
2 files changed, 156 insertions(+), 11 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] qemu: fix domain start with corrupted save file

[Peter Krempa]

On Wed, Apr 22, 2020 at 12:54:26 +0200, Pavel Mores wrote:
> This is to fix
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791522

Please describe the bug in the commit message rather than have users
open and try understand external links.


> 
> With this change, if a domain comes across a corrupted save file during boot 
> it
> removes the save file and logs a warning but continues to boot normally 
> instead
> of failing to boot (with a subsequent boot attempt succeeding).
> 
> The regression was introduced by 21ad56e932 and this change effectively 
> reverts
> the relevant part of that commit.

Please break commit messages at 72 colums as it's common.


> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e8d47a41cd..2579ef3984 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  *ret_def = def;
>  *ret_data = data;
>  
> + cleanup:
>  return fd;
>  
>   error:
>  virDomainDefFree(def);
>  virQEMUSaveDataFree(data);
>  VIR_FORCE_CLOSE(fd);
> -return -1;
> +goto cleanup;

I think this should be fixed properly. 'fd' is serving double duty of
also reporting the erorrs here and it's not really obvious from this
hunk. The code above sets 'fd' to a variety of values e.g. -3 on some
errors. This is really tricky and hard to follow.

In this case we want a 'ret' variable which is set to the proper value
and returned on error and fd returned on success or alternatively
assigned to 'ret' right before returning it. This should make it
possible to do it even without adding new not really useful labels.

Re: [PATCH v2 1/2] conf: Add option for settings

[Daniel Henrique Barboza]

On 4/21/20 1:03 AM, Julio Faracco wrote:

If an user is trying to configure a dhcp neetwork settings, it is not


s/neetwork/network



This patch failed to compile in my box on top of master at 9a13704818e:


  CCLD libvirdeterministichashmock.la
../../tests/networkxml2conftest.c: In function 'testCompareXMLToConfFiles':
../../tests/networkxml2conftest.c:46:59: error: passing argument 4 of 
'networkDnsmasqConfContents' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, caps) < 
0)
  |   ^~~~
  |   |
  |   dnsmasqContext * 
{aka struct  *}
In file included from ../../tests/networkxml2conftest.c:13:
../../src/network/bridge_driver.h:49:35: note: expected 'char **' but argument is of 
type 'dnsmasqContext *' {aka 'struct  *'}
   49 |char **hostsfilestr,
  |~~~^~~~
../../tests/networkxml2conftest.c:46:65: error: passing argument 5 of 
'networkDnsmasqConfContents' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, caps) < 
0)
  | ^~~~
  | |
  | 
dnsmasqCapsPtr {aka struct _dnsmasqCaps *}
In file included from ../../tests/networkxml2conftest.c:13:
../../src/network/bridge_driver.h:50:44: note: expected 'dnsmasqContext *' {aka 
'struct  *'} but argument is of type 'dnsmasqCapsPtr' {aka 'struct 
_dnsmasqCaps *'}
   50 |dnsmasqContext *dctx,
  |^~~~
../../tests/networkxml2conftest.c:46:9: error: too few arguments to function 
'networkDnsmasqConfContents'
   46 | if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, caps) < 
0)
  | ^~
In file included from ../../tests/networkxml2conftest.c:13:
../../src/network/bridge_driver.h:46:1: note: declared here
   46 | networkDnsmasqConfContents(virNetworkObjPtr obj,
  | ^~
cc1: all warnings being treated as errors




Thanks,


DHB

[libvirt PATCH] qemu: fix domain start with corrupted save file

[Pavel Mores]

This is to fix

https://bugzilla.redhat.com/show_bug.cgi?id=1791522

With this change, if a domain comes across a corrupted save file during boot it
removes the save file and logs a warning but continues to boot normally instead
of failing to boot (with a subsequent boot attempt succeeding).

The regression was introduced by 21ad56e932 and this change effectively reverts
the relevant part of that commit.
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8d47a41cd..2579ef3984 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 *ret_def = def;
 *ret_data = data;
 
+ cleanup:
 return fd;
 
  error:
 virDomainDefFree(def);
 virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
-return -1;
+goto cleanup;
 }
 
 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.24.1

[PATCH] qemucapstest: Refresh data for qemu 5.0 on x86_64

[Peter Krempa]

Bump to v5.0.0-rc3-8-g3119154db0 and make sure that 'liburing' is picked
up by qemu.

Signed-off-by: Peter Krempa 
---
 .../caps_5.0.0.x86_64.replies | 99 +--
 .../caps_5.0.0.x86_64.xml | 68 -
 2 files changed, 156 insertions(+), 11 deletions(-)

diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies 
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
index af2299a41e..39b6d4f182 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
@@ -17,11 +17,11 @@
 {
   "return": {
 "qemu": {
-  "micro": 91,
+  "micro": 93,
   "minor": 2,
   "major": 4
 },
-"package": "v5.0.0-rc1"
+"package": "v5.0.0-rc3-8-g3119154db0"
   },
   "id": "libvirt-2"
 }
@@ -882,6 +882,10 @@
   "name": "tpm-passthrough",
   "parent": "tpm-backend"
 },
+{
+  "name": "Icelake-Server-v3-x86_64-cpu",
+  "parent": "x86_64-cpu"
+},
 {
   "name": "pentium3-v1-x86_64-cpu",
   "parent": "x86_64-cpu"
@@ -1291,8 +1295,8 @@
   "parent": "base-xhci"
 },
 {
-  "name": "pc-i440fx-2.6-machine",
-  "parent": "generic-pc-machine"
+  "name": "mioe3680_pci",
+  "parent": "pci-device"
 },
 {
   "name": "i82551",
@@ -1315,11 +1319,11 @@
   "parent": "pci-ide"
 },
 {
-  "name": "nvme",
-  "parent": "pci-device"
+  "name": "pc-i440fx-2.6-machine",
+  "parent": "generic-pc-machine"
 },
 {
-  "name": "vmxnet3",
+  "name": "nvme",
   "parent": "pci-device"
 },
 {
@@ -1327,7 +1331,7 @@
   "parent": "object"
 },
 {
-  "name": "mioe3680_pci",
+  "name": "vmxnet3",
   "parent": "pci-device"
 },
 {
@@ -9055,6 +9059,39 @@
   "static": false,
   "migration-safe": true
 },
+{
+  "name": "Icelake-Server-v3",
+  "typename": "Icelake-Server-v3-x86_64-cpu",
+  "unavailable-features": [
+"avx512f",
+"avx512dq",
+"clwb",
+"avx512cd",
+"avx512bw",
+"avx512vl",
+"avx512vbmi",
+"pku",
+"avx512vbmi2",
+"gfni",
+"vaes",
+"vpclmulqdq",
+"avx512vnni",
+"avx512bitalg",
+"avx512-vpopcntdq",
+"la57",
+"wbnoinvd",
+"avx512f",
+"avx512f",
+"avx512f",
+"pku",
+"rdctl-no",
+"ibrs-all",
+"mds-no",
+"taa-no"
+  ],
+  "static": false,
+  "migration-safe": true
+},
 {
   "name": "Icelake-Server-v2",
   "typename": "Icelake-Server-v2-x86_64-cpu",
@@ -22244,7 +22281,8 @@
   "meta-type": "enum",
   "values": [
 "threads",
-"native"
+"native",
+"io_uring"
   ]
 },
 {
@@ -26527,6 +26565,49 @@
   "static": false,
   "migration-safe": true
 },
+{
+  "name": "Icelake-Server-v3",
+  "typename": "Icelake-Server-v3-x86_64-cpu",
+  "unavailable-features": [
+"fma",
+"pcid",
+"x2apic",
+"tsc-deadline",
+"avx",
+"f16c",
+"avx2",
+"invpcid",
+"avx512f",
+"avx512dq",
+"rdseed",
+"avx512cd",
+"avx512bw",
+"avx512vl",
+"avx512vbmi",
+"umip",
+"avx512vbmi2",
+"gfni",
+"vaes",
+"vpclmulqdq",
+"avx512vnni",
+"avx512bitalg",
+"avx512-vpopcntdq",
+"spec-ctrl",
+"arch-capabilities",
+"ssbd",
+"3dnowprefetch",
+"wbnoinvd",
+"xsavec",
+"rdctl-no",
+"ibrs-all",
+"skip-l1dfl-vmentry",
+"mds-no",
+"pschange-mc-no",
+"taa-no"
+  ],
+  "static": false,
+  "migration-safe": true
+},
 {
   "name": "Icelake-Server-v2",
   "typename": "Icelake-Server-v2-x86_64-cpu",
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index 496d0e90d7..d524340843 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -230,10 +230,10 @@
   
   
   
-  4002091
+  4002093
   0
   43100241
-  v5.0.0-rc1
+  v5.0.0-rc3-8-g3119154db0
   x86_64
   
 
@@ -780,6 +780,33 @@
   
   
   
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
   
 
 
@@ -1978,6 +2005,43 @@
 
 
   
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
   
 
 
-- 
2.25.1

Re: [libvirt-python PATCH v2] setup: require python >= 3.5 to build

[Daniel P . Berrangé]

On Tue, Apr 21, 2020 at 10:23:45PM +0200, Philipp Hahn wrote:
> Am 20.04.20 um 15:57 schrieb Daniel P. Berrangé:
> > Pytjon 3.5 is the oldest Python version available across our supported

I'll fix this typo before pushing

> > build platforms.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> 
> Reviewed-by: Philipp Hahn 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: Re: [RFC 00/29] RFC: Generate object-model code based on relax-ng files

[Daniel P . Berrangé]

On Wed, Apr 22, 2020 at 11:37:23AM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 09:48 +0100, Daniel P. Berrangé wrote:
> > On Wed, Apr 22, 2020 at 01:51:10PM +0800, Shi Lei wrote:
> > > As you suggested, parsing C-structs plus some magic comments may be a 
> > > good starting point.
> > > But parsing C is much more difficult than parsing xml or json. I think 
> > > this job should base on
> > > some present tools or other basis. We can first refer to Clang or other 
> > > light compiler front-end.
> > > Maybe we can utilize some output of the middle stage of these compilers.
> > 
> > FWIW, we already have a tool in libvirt that parsers C header files. The
> > scripts/apibuild.py file extracts info about our enums, structs, APIs and
> > uses it to build the public API documentation.
> > 
> > I wonder if we can refactor that tool to extract the code for parsing into
> > a module, so that we can more reasily re-use it for both the API docs and
> > for a new XML generator
> 
> Or we could replace that C parsing code with something based on
> libclang.

I think that's a double edged sword. While it gives you the full coverage
of the C language, you then have to deal with the full range of the C
language. I think the simplified parser we have for the docs build will
be easier to work with by being more constrained in what it tries to
support.

> Either way, if this ever becomes usable I think it should not live
> in the libvirt repository but be a standalone tool instead, as I can
> see many projects potentially benefiting from it.

Yes, but we can worry about that at a later date IMHO.

> Which begs the question: are we absolutely certain something like
> this doesn't exist already? We should make sure that's really the
> case before we invest time on it...

I've not found anything equivalent to Golang's XML parser for C

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 2/3] node_device_udev: Split udevRemoveOneDevice() into two

[Martin Kletzander]

On Mon, Apr 20, 2020 at 04:20:40PM +0200, Michal Privoznik wrote:

Move internals of udevRemoveOneDevice() into a separate function
which accepts sysfs path as an argument and actually removes the
device from the internal list. It will be reused later.

Signed-off-by: Michal Privoznik 


Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [PATCH 3/3] udevHandleOneDevice: Remove old instance of device on "move"

[Martin Kletzander]

On Mon, Apr 20, 2020 at 04:20:41PM +0200, Michal Privoznik wrote:

When a device is "move"-d (this basically means it was renamed),
we add the new device onto our list but keep the old there too.
Fortunately, udev sets this DEVPATH_OLD property which points to
the old device path. We can use it to remove the old instance.

To test this try renaming an interface, for instance:

 # ip link set tunl0 name tunl1
 # ip link set tunl1 name tunl0

One problem with udev is that it sends old ifname in INTERFACE
property, which creates a problem for us, the property is where
we get the ifname from and use it then to query all kind of info
about the interface. Well, if it is non-existent then we can't
query anything. This happens if ifname rename is suppressed
(net.ifnames=0 on kernel cmd line for instance). Fortunately, we
can use "kernel" source for udev events which has always the
fresh info.

Signed-off-by: Michal Privoznik 


Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: Re: [RFC 00/29] RFC: Generate object-model code based on relax-ng files

[Andrea Bolognani]

On Wed, 2020-04-22 at 09:48 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 01:51:10PM +0800, Shi Lei wrote:
> > As you suggested, parsing C-structs plus some magic comments may be a good 
> > starting point.
> > But parsing C is much more difficult than parsing xml or json. I think this 
> > job should base on
> > some present tools or other basis. We can first refer to Clang or other 
> > light compiler front-end.
> > Maybe we can utilize some output of the middle stage of these compilers.
> 
> FWIW, we already have a tool in libvirt that parsers C header files. The
> scripts/apibuild.py file extracts info about our enums, structs, APIs and
> uses it to build the public API documentation.
> 
> I wonder if we can refactor that tool to extract the code for parsing into
> a module, so that we can more reasily re-use it for both the API docs and
> for a new XML generator

Or we could replace that C parsing code with something based on
libclang.

Either way, if this ever becomes usable I think it should not live
in the libvirt repository but be a standalone tool instead, as I can
see many projects potentially benefiting from it.

Which begs the question: are we absolutely certain something like
this doesn't exist already? We should make sure that's really the
case before we invest time on it...

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: Re: [RFC 00/29] RFC: Generate object-model code based on relax-ng files

[Shi Lei]

>On Wed, Apr 22, 2020 at 01:51:10PM +0800, Shi Lei wrote:
>> As you suggested, parsing C-structs plus some magic comments may be a good 
>> starting point.
>> But parsing C is much more difficult than parsing xml or json. I think this 
>> job should base on
>> some present tools or other basis. We can first refer to Clang or other 
>> light compiler front-end.
>> Maybe we can utilize some output of the middle stage of these compilers.
>
>FWIW, we already have a tool in libvirt that parsers C header files. The
>scripts/apibuild.py file extracts info about our enums, structs, APIs and
>uses it to build the public API documentation. 

Okay. I'll examine it.

>
>I wonder if we can refactor that tool to extract the code for parsing into
>a module, so that we can more reasily re-use it for both the API docs and
>for a new XML generator 

It makes sense if this tool is suitable.

Regards,
Shi Lei

>
>Regards,
>Daniel
>--
>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org -o- https://fstop138.berrange.com :|
>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>

Re: [PATCH v2 1/5] qemu_capabilities: Introduce QEMU_CAPS_AIO_IO_URING

[Han Han]

OK. Get it

On Wed, Apr 22, 2020 at 4:49 PM Peter Krempa  wrote:

> On Wed, Apr 22, 2020 at 16:28:06 +0800, Han Han wrote:
> > On Wed, Apr 22, 2020 at 4:14 PM Peter Krempa  wrote:
> >
> > > On Tue, Apr 21, 2020 at 20:19:34 +0800, Han Han wrote:
> > > > Add io_uring value to capability replies.
> > > >
> > > > The capability QEMU_CAPS_AIO_IO_URING will be used for io_uring aio
> mode,
> > > > introduced from QEMU 5.0, linux 5.1.
> > > >
> > > > Signed-off-by: Han Han 
> > > > ---
> > > >  src/qemu/qemu_capabilities.c  | 2 ++
> > > >  src/qemu/qemu_capabilities.h  | 1 +
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies | 3 ++-
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.replies   | 3 ++-
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies  | 3 ++-
> > > >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
> > > >  8 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > [...]
> > >
> > > > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > > index af2299a4..970e483e 100644
> > > > --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > > @@ -22244,7 +22244,8 @@
> > > >"meta-type": "enum",
> > > >"values": [
> > > >  "threads",
> > > > -"native"
> > > > +"native",
> > > > +"io_uring"
> > >
> > > Did you patch this manually?
> > >
> > > Yes
>
> Okay, please don't do that in the future. The idea of the 'replies'
> files is that they are captured from real qemu. I've currently modified
> my qemu install to contain liburing so I'll post the updated ones.
>
> I'll also drop any other hunks related to 'replies' in this patch when
> I'll be pushing this.
>
>

-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333

Re: [PATCH v2 1/5] qemu_capabilities: Introduce QEMU_CAPS_AIO_IO_URING

[Peter Krempa]

On Wed, Apr 22, 2020 at 16:28:06 +0800, Han Han wrote:
> On Wed, Apr 22, 2020 at 4:14 PM Peter Krempa  wrote:
> 
> > On Tue, Apr 21, 2020 at 20:19:34 +0800, Han Han wrote:
> > > Add io_uring value to capability replies.
> > >
> > > The capability QEMU_CAPS_AIO_IO_URING will be used for io_uring aio mode,
> > > introduced from QEMU 5.0, linux 5.1.
> > >
> > > Signed-off-by: Han Han 
> > > ---
> > >  src/qemu/qemu_capabilities.c  | 2 ++
> > >  src/qemu/qemu_capabilities.h  | 1 +
> > >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies | 3 ++-
> > >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
> > >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.replies   | 3 ++-
> > >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
> > >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies  | 3 ++-
> > >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
> > >  8 files changed, 12 insertions(+), 3 deletions(-)
> >
> > [...]
> >
> > > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > index af2299a4..970e483e 100644
> > > --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > > @@ -22244,7 +22244,8 @@
> > >"meta-type": "enum",
> > >"values": [
> > >  "threads",
> > > -"native"
> > > +"native",
> > > +"io_uring"
> >
> > Did you patch this manually?
> >
> > Yes

Okay, please don't do that in the future. The idea of the 'replies'
files is that they are captured from real qemu. I've currently modified
my qemu install to contain liburing so I'll post the updated ones.

I'll also drop any other hunks related to 'replies' in this patch when
I'll be pushing this.

Re: Re: [RFC 00/29] RFC: Generate object-model code based on relax-ng files

[Daniel P . Berrangé]

On Wed, Apr 22, 2020 at 01:51:10PM +0800, Shi Lei wrote:
> As you suggested, parsing C-structs plus some magic comments may be a good 
> starting point.
> But parsing C is much more difficult than parsing xml or json. I think this 
> job should base on
> some present tools or other basis. We can first refer to Clang or other light 
> compiler front-end.
> Maybe we can utilize some output of the middle stage of these compilers.

FWIW, we already have a tool in libvirt that parsers C header files. The
scripts/apibuild.py file extracts info about our enums, structs, APIs and
uses it to build the public API documentation.

I wonder if we can refactor that tool to extract the code for parsing into
a module, so that we can more reasily re-use it for both the API docs and
for a new XML generator

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] virsh: Fix return code for dump and migrate

[Ján Tomko]

On a Wednesday in 2020, Andrea Bolognani wrote:

On Tue, 2020-04-21 at 18:25 +0100, Daniel P. Berrangé wrote:

On Tue, Apr 21, 2020 at 07:15:00PM +0200, Andrea Bolognani wrote:
> When the job monitoring logic was refactored, these two commands
> were not converted properly and the result is that a successful
> dump or migration (char '0') would be reported as a failed one
> (int 48) instead.
>
> Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
> Reported-by: Brian Rak 
> Signed-off-by: Andrea Bolognani 
> ---
>  tools/virsh-domain.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Thanks, pushed now. And thanks again to Brian for reporting the issue
and tracking it down!

Do you think we should backport this to the 6.1 and 6.2 mainteinance
branches? It fixes a fairly bad regression... But I'm not very
familiar with how we handle those branches.



We don't because nobody was using them anymore.

Jano


--
Andrea Bolognani / Red Hat / Virtualization



signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] qemu_capabilities: Introduce QEMU_CAPS_AIO_IO_URING

[Han Han]

On Wed, Apr 22, 2020 at 4:14 PM Peter Krempa  wrote:

> On Tue, Apr 21, 2020 at 20:19:34 +0800, Han Han wrote:
> > Add io_uring value to capability replies.
> >
> > The capability QEMU_CAPS_AIO_IO_URING will be used for io_uring aio mode,
> > introduced from QEMU 5.0, linux 5.1.
> >
> > Signed-off-by: Han Han 
> > ---
> >  src/qemu/qemu_capabilities.c  | 2 ++
> >  src/qemu/qemu_capabilities.h  | 1 +
> >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies | 3 ++-
> >  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
> >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.replies   | 3 ++-
> >  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
> >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies  | 3 ++-
> >  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
> >  8 files changed, 12 insertions(+), 3 deletions(-)
>
> [...]
>
> > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > index af2299a4..970e483e 100644
> > --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> > @@ -22244,7 +22244,8 @@
> >"meta-type": "enum",
> >"values": [
> >  "threads",
> > -"native"
> > +"native",
> > +"io_uring"
>
> Did you patch this manually?
>
> Yes

> >]
> >  },
> >  {
>
> I'll update the 5.0.0 qemu caps first so that we can push it without
> manual intervetntion.
>
> OK. Wait for your caps patch :)


-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333

Re: [PATCH v2 1/5] qemu_capabilities: Introduce QEMU_CAPS_AIO_IO_URING

[Peter Krempa]

On Tue, Apr 21, 2020 at 20:19:34 +0800, Han Han wrote:
> Add io_uring value to capability replies.
> 
> The capability QEMU_CAPS_AIO_IO_URING will be used for io_uring aio mode,
> introduced from QEMU 5.0, linux 5.1.
> 
> Signed-off-by: Han Han 
> ---
>  src/qemu/qemu_capabilities.c  | 2 ++
>  src/qemu/qemu_capabilities.h  | 1 +
>  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies | 3 ++-
>  tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.replies   | 3 ++-
>  tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies  | 3 ++-
>  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
>  8 files changed, 12 insertions(+), 3 deletions(-)

[...]

> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies 
> b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> index af2299a4..970e483e 100644
> --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
> @@ -22244,7 +22244,8 @@
>"meta-type": "enum",
>"values": [
>  "threads",
> -"native"
> +"native",
> +"io_uring"

Did you patch this manually?

>]
>  },
>  {

I'll update the 5.0.0 qemu caps first so that we can push it without
manual intervetntion.

Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error

[Michal Privoznik]

On 4/21/20 8:10 PM, Marc-André Lureau wrote:


Oh ok, that should be fine.
thanks



Cool! I've made the change and pushed. To all:

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] news: Document new Xen hypervisor features

[Andrea Bolognani]

On Tue, 2020-04-21 at 16:06 -0600, Jim Fehlig wrote:
> +
> +  Xen: add support for e820_host hypervisor feature

s/Xen: add/xen: Add/

I suggest also putting either single or double quotes around the name
of the feature for clarity.

> +
> +
> +  e820_host is a Xen-specific option only available for PV guests.

And here you could wrap it in a  element so that it will
display nicely on the Web.

These comments apply also to the second item.

Thanks for taking care of the release notes!


Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration

[Yan Zhao]

On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote:
> > From: Yan Zhao
> > Sent: Tuesday, April 21, 2020 10:37 AM
> > 
> > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson wrote:
> > > On Sun, 19 Apr 2020 21:24:57 -0400
> > > Yan Zhao  wrote:
> > >
> > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck wrote:
> > > > > On Fri, 17 Apr 2020 05:52:02 -0400
> > > > > Yan Zhao  wrote:
> > > > >
> > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck wrote:
> > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400
> > > > > > > Yan Zhao  wrote:
> > > > > > >
> > > > > > > > This patchset introduces a migration_version attribute under 
> > > > > > > > sysfs
> > of VFIO
> > > > > > > > Mediated devices.
> > > > > > > >
> > > > > > > > This migration_version attribute is used to check migration
> > compatibility
> > > > > > > > between two mdev devices.
> > > > > > > >
> > > > > > > > Currently, it has two locations:
> > > > > > > > (1) under mdev_type node,
> > > > > > > > which can be used even before device creation, but only for
> > mdev
> > > > > > > > devices of the same mdev type.
> > > > > > > > (2) under mdev device node,
> > > > > > > > which can only be used after the mdev devices are created, 
> > > > > > > > but
> > the src
> > > > > > > > and target mdev devices are not necessarily be of the same
> > mdev type
> > > > > > > > (The second location is newly added in v5, in order to keep
> > consistent
> > > > > > > > with the migration_version node for migratable pass-though
> > devices)
> > > > > > >
> > > > > > > What is the relationship between those two attributes?
> > > > > > >
> > > > > > (1) is for mdev devices specifically, and (2) is provided to keep 
> > > > > > the
> > same
> > > > > > sysfs interface as with non-mdev cases. so (2) is for both mdev
> > devices and
> > > > > > non-mdev devices.
> > > > > >
> > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a non-mdev device
> > > > > > is binding to vfio-pci, but is able to register migration region 
> > > > > > and do
> > > > > > migration transactions from a vendor provided affiliate driver),
> > > > > > the vendor driver would export (2) directly, under device node.
> > > > > > It is not able to provide (1) as there're no mdev devices involved.
> > > > >
> > > > > Ok, creating an alternate attribute for non-mdev devices makes sense.
> > > > > However, wouldn't that rather be a case (3)? The change here only
> > > > > refers to mdev devices.
> > > > >
> > > > as you pointed below, (3) and (2) serve the same purpose.
> > > > and I think a possible usage is to migrate between a non-mdev device and
> > > > an mdev device. so I think it's better for them both to use (2) rather
> > > > than creating (3).
> > >
> > > An mdev type is meant to define a software compatible interface, so in
> > > the case of mdev->mdev migration, doesn't migrating to a different type
> > > fail the most basic of compatibility tests that we expect userspace to
> > > perform?  IOW, if two mdev types are migration compatible, it seems a
> > > prerequisite to that is that they provide the same software interface,
> > > which means they should be the same mdev type.
> > >
> > > In the hybrid cases of mdev->phys or phys->mdev, how does a
> > management
> > > tool begin to even guess what might be compatible?  Are we expecting
> > > libvirt to probe ever device with this attribute in the system?  Is
> > > there going to be a new class hierarchy created to enumerate all
> > > possible migrate-able devices?
> > >
> > yes, management tool needs to guess and test migration compatible
> > between two devices. But I think it's not the problem only for
> > mdev->phys or phys->mdev. even for mdev->mdev, management tool needs
> > to
> > first assume that the two mdevs have the same type of parent devices
> > (e.g.their pciids are equal). otherwise, it's still enumerating
> > possibilities.
> > 
> > on the other hand, for two mdevs,
> > mdev1 from pdev1, its mdev_type is 1/2 of pdev1;
> > mdev2 from pdev2, its mdev_type is 1/4 of pdev2;
> > if pdev2 is exactly 2 times of pdev1, why not allow migration between
> > mdev1 <-> mdev2.
> 
> How could the manage tool figure out that 1/2 of pdev1 is equivalent 
> to 1/4 of pdev2? If we really want to allow such thing happen, the best
> choice is to report the same mdev type on both pdev1 and pdev2.
I think that's exactly the value of this migration_version interface.
the management tool can take advantage of this interface to know if two
devices are migration compatible, no matter they are mdevs, non-mdevs,
or mix.

as I know, (please correct me if not right), current libvirt still
requires manually generating mdev devices, and it just duplicates src vm
configuration to the target vm.
for libvirt, currently it's always phys->phys and mdev->mdev (and of the
same mdev type).
But it does not justify that hybrid cases should not be allowed. otherwise,
why do we need to introduce this migra

Re: [libvirt PATCH] virsh: Fix return code for dump and migrate

[Andrea Bolognani]

On Tue, 2020-04-21 at 18:25 +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 21, 2020 at 07:15:00PM +0200, Andrea Bolognani wrote:
> > When the job monitoring logic was refactored, these two commands
> > were not converted properly and the result is that a successful
> > dump or migration (char '0') would be reported as a failed one
> > (int 48) instead.
> > 
> > Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
> > Reported-by: Brian Rak 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  tools/virsh-domain.c | 10 +++---
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé 

Thanks, pushed now. And thanks again to Brian for reporting the issue
and tracking it down!

Do you think we should backport this to the 6.1 and 6.2 mainteinance
branches? It fixes a fairly bad regression... But I'm not very
familiar with how we handle those branches.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[Zhenyu Zheng]

 gitlab CI testing as suggested:
https://gitlab.com/ZhengZhenyu/libvirt/pipelines/134657317

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Introduce getHost support for ARM CPU driver, read
> CPU vendor_id, part_id and flags from registers
> directly. These codes will only be compiled on
> aarch64 hardwares.
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 204 ++
>  1 file changed, 204 insertions(+)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 6e9ff9bf11..ec50a5615d 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -21,6 +21,10 @@
>   */
>
>  #include 
> +#if defined(__aarch64__)
> +# include 
> +# include 
> +#endif
>
>  #include "viralloc.h"
>  #include "cpu.h"
> @@ -32,6 +36,15 @@
>
>  #define VIR_FROM_THIS VIR_FROM_CPU
>
> +#if defined(__aarch64__)
> +/* Shift bit mask for parsing cpu flags */
> +# define BIT_SHIFTS(n) (1UL << (n))
> +/* The current max number of cpu flags on ARM is 32 */
> +# define MAX_CPU_FLAGS 32
> +#endif
> +
> +VIR_LOG_INIT("cpu.cpu_arm");
> +
>  static const virArch archs[] = {
>  VIR_ARCH_ARMV6L,
>  VIR_ARCH_ARMV7B,
> @@ -491,12 +504,203 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
>  return 0;
>  }
>
> +#if defined(__aarch64__)
> +/**
> + * virCPUarmCpuDataFromRegs:
> + *
> + * @data: 64-bit arm CPU specific data
> + *
> + * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU
> + * flags from AT_HWCAP. There are currently 32 valid flags  on ARM arch
> + * represented by each bit.
> + */
> +static int
> +virCPUarmCpuDataFromRegs(virCPUarmData *data)
> +{
> +/* Generate human readable flag list according to the order of */
> +/* AT_HWCAP bit map */
> +const char *flag_list[MAX_CPU_FLAGS] = {
> +"fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",
> +"crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",
> +"jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",
> +"asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",
> +"ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};
> +unsigned long cpuid, hwcaps;
> +char **features = NULL;
> +g_autofree char *cpu_feature_str = NULL;
> +int cpu_feature_index = 0;
> +size_t i;
> +
> +if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("CPUID registers unavailable"));
> +return -1;
> +}
> +
> +/* read the cpuid data from MIDR_EL1 register */
> +asm("mrs %0, MIDR_EL1" : "=r" (cpuid));
> +VIR_DEBUG("CPUID read from register:  0x%016lx", cpuid);
> +
> +/* parse the coresponding part_id bits */
> +data->pvr = cpuid>>4&0xFFF;
> +/* parse the coresponding vendor_id bits */
> +data->vendor_id = cpuid>>24&0xFF;
> +
> +hwcaps = getauxval(AT_HWCAP);
> +VIR_DEBUG("CPU flags read from register:  0x%016lx", hwcaps);
> +
> +if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)
> +return -1;
> +
> +/* shift bit map mask to parse for CPU flags */
> +for (i = 0; i< MAX_CPU_FLAGS; i++) {
> +if (hwcaps & BIT_SHIFTS(i)) {
> +features[cpu_feature_index] = g_strdup(flag_list[i]);
> +cpu_feature_index++;
> +}
> +}
> +
> +if (cpu_feature_index > 1) {
> +cpu_feature_str = virStringListJoin((const char **)features, " ");
> +if (!cpu_feature_str)
> +goto error;
> +}
> +data->features = g_strdup(cpu_feature_str);
> +
> +return 0;
> +
> + error:
> +virStringListFree(features);
> +return -1;
> +}
> +
> +static int
> +virCPUarmDataParseFeatures(virCPUDefPtr cpu,
> +   const virCPUarmData *cpuData)
> +{
> +int ret = -1;
> +size_t i;
> +char **features;
> +
> +if (!cpu || !cpuData)
> +return ret;
> +
> +if (!(features = virStringSplitCount(cpuData->features, " ",
> + 0, &cpu->nfeatures)))
> +return ret;
> +if (cpu->nfeatures) {
> +if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)
> +goto error;
> +
> +for (i = 0; i < cpu->nfeatures; i++) {
> +cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> +cpu->features[i].name = g_strdup(features[i]);
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virStringListFree(features);
> +return ret;
> +
> + error:
> +for (i = 0; i < cpu->nfeatures; i++)
> +VIR_FREE(cpu->features[i].name);
> +VIR_FREE(cpu->features);
> +cpu->nfeatures = 0;
> +goto cleanup;
> +}
> +
> +static int
> +virCPUarmDecode(virCPUDefPtr cpu,
> +const virCPUarmData *cpuData,
> +virDomainCapsCPUModelsPtr models)
> +{
> +virCPUarmMapPtr map;
> +virCPUarmModelPtr model;
> +virCPUarmVendorPtr vendor = NULL;
> +
> +if (!cpuData || !(map = virCPUarmGetMap()))
> +return -1;
> +
> 

[PATCH V3 3/5] cpu: Introduce ARM related structs

[ZhengZhenyu]

Introduce vendor and model struct and related
cleanup functions for ARM cpu.

Signed-off-by: Zhenyu Zheng 
---
 src/cpu/cpu_arm.c | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index ee5802198f..2009904cc9 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -1,6 +1,7 @@
 /*
  * cpu_arm.c: CPU driver for arm CPUs
  *
+ * Copyright (C) 2020 Huawei Technologies Co., Ltd.
  * Copyright (C) 2013 Red Hat, Inc.
  * Copyright (C) Canonical Ltd. 2012
  *
@@ -23,7 +24,9 @@
 
 #include "viralloc.h"
 #include "cpu.h"
+#include "cpu_arm.h"
 #include "cpu_map.h"
+#include "virlog.h"
 #include "virstring.h"
 #include "virxml.h"
 
@@ -36,6 +39,21 @@ static const virArch archs[] = {
 VIR_ARCH_AARCH64,
 };
 
+typedef struct _virCPUarmVendor virCPUarmVendor;
+typedef virCPUarmVendor *virCPUarmVendorPtr;
+struct _virCPUarmVendor {
+char *name;
+unsigned long value;
+};
+
+typedef struct _virCPUarmModel virCPUarmModel;
+typedef virCPUarmModel *virCPUarmModelPtr;
+struct _virCPUarmModel {
+char *name;
+virCPUarmVendorPtr vendor;
+virCPUarmData data;
+};
+
 typedef struct _virCPUarmFeature virCPUarmFeature;
 typedef virCPUarmFeature *virCPUarmFeaturePtr;
 struct _virCPUarmFeature {
@@ -64,6 +82,10 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUarmFeature, 
virCPUarmFeatureFree);
 typedef struct _virCPUarmMap virCPUarmMap;
 typedef virCPUarmMap *virCPUarmMapPtr;
 struct _virCPUarmMap {
+size_t nvendors;
+virCPUarmVendorPtr *vendors;
+size_t nmodels;
+virCPUarmModelPtr *models;
 GPtrArray *features;
 };
 
@@ -81,12 +103,62 @@ virCPUarmMapNew(void)
 return map;
 }
 
+static void
+virCPUarmDataClear(virCPUarmData *data)
+{
+if (!data)
+return;
+
+VIR_FREE(data->features);
+}
+
+static void
+virCPUarmDataFree(virCPUDataPtr cpuData)
+{
+if (!cpuData)
+return;
+
+virCPUarmDataClear(&cpuData->data.arm);
+VIR_FREE(cpuData);
+}
+
+static void
+virCPUarmModelFree(virCPUarmModelPtr model)
+{
+if (!model)
+return;
+
+virCPUarmDataClear(&model->data);
+g_free(model->name);
+g_free(model);
+}
+
+static void
+virCPUarmVendorFree(virCPUarmVendorPtr vendor)
+{
+if (!vendor)
+return;
+
+g_free(vendor->name);
+g_free(vendor);
+}
+
 static void
 virCPUarmMapFree(virCPUarmMapPtr map)
 {
 if (!map)
 return;
 
+size_t i;
+
+for (i = 0; i < map->nmodels; i++)
+virCPUarmModelFree(map->models[i]);
+g_free(map->models);
+
+for (i = 0; i < map->nvendors; i++)
+virCPUarmVendorFree(map->vendors[i]);
+g_free(map->vendors);
+
 g_ptr_array_free(map->features, TRUE);
 
 g_free(map);
@@ -259,6 +331,7 @@ struct cpuArchDriver cpuDriverArm = {
 .compare = virCPUarmCompare,
 .decode = NULL,
 .encode = NULL,
+.dataFree = virCPUarmDataFree,
 .baseline = virCPUarmBaseline,
 .update = virCPUarmUpdate,
 .validateFeatures = virCPUarmValidateFeatures,
-- 
2.26.0.windows.1

[PATCH V3 2/5] cpu: Introduce virCPUarmData to virCPUData

[ZhengZhenyu]

Introduce virCPUarmData to virCPUData

Signed-off-by: Zhenyu Zheng 
---
 src/cpu/Makefile.inc.am |  1 +
 src/cpu/cpu.h   |  2 ++
 src/cpu/cpu_arm_data.h  | 31 +++
 3 files changed, 34 insertions(+)
 create mode 100644 src/cpu/cpu_arm_data.h

diff --git a/src/cpu/Makefile.inc.am b/src/cpu/Makefile.inc.am
index 0abeee87b6..bea203fb5c 100644
--- a/src/cpu/Makefile.inc.am
+++ b/src/cpu/Makefile.inc.am
@@ -10,6 +10,7 @@ CPU_SOURCES = \
cpu/cpu_s390.c \
cpu/cpu_arm.h \
cpu/cpu_arm.c \
+   cpu/cpu_arm_data.h \
cpu/cpu_ppc64.h \
cpu/cpu_ppc64.c \
cpu/cpu_ppc64_data.h \
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index f779d2be17..ec22a183a1 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -27,6 +27,7 @@
 #include "cpu_conf.h"
 #include "cpu_x86_data.h"
 #include "cpu_ppc64_data.h"
+#include "cpu_arm_data.h"
 
 
 typedef struct _virCPUData virCPUData;
@@ -36,6 +37,7 @@ struct _virCPUData {
 union {
 virCPUx86Data x86;
 virCPUppc64Data ppc64;
+virCPUarmData arm;
 /* generic driver needs no data */
 } data;
 };
diff --git a/src/cpu/cpu_arm_data.h b/src/cpu/cpu_arm_data.h
new file mode 100644
index 00..cf12ca8c2e
--- /dev/null
+++ b/src/cpu/cpu_arm_data.h
@@ -0,0 +1,31 @@
+/*
+ * cpu_arm_data.h: 64-bit arm CPU specific data
+ *
+ * Copyright (C) 2020 Huawei Technologies Co., Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library;  If not, see
+ * .
+ *
+ */
+
+#pragma once
+
+#define VIR_CPU_ARM_DATA_INIT { 0 }
+
+typedef struct _virCPUarmData virCPUarmData;
+struct _virCPUarmData {
+unsigned long vendor_id;
+unsigned long pvr;
+char *features;
+};
-- 
2.26.0.windows.1

[PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[ZhengZhenyu]

Introduce getHost support for ARM CPU driver, read
CPU vendor_id, part_id and flags from registers
directly. These codes will only be compiled on
aarch64 hardwares.

Signed-off-by: Zhenyu Zheng 
---
 src/cpu/cpu_arm.c | 204 ++
 1 file changed, 204 insertions(+)

diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index 6e9ff9bf11..ec50a5615d 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -21,6 +21,10 @@
  */
 
 #include 
+#if defined(__aarch64__)
+# include 
+# include 
+#endif
 
 #include "viralloc.h"
 #include "cpu.h"
@@ -32,6 +36,15 @@
 
 #define VIR_FROM_THIS VIR_FROM_CPU
 
+#if defined(__aarch64__)
+/* Shift bit mask for parsing cpu flags */
+# define BIT_SHIFTS(n) (1UL << (n))
+/* The current max number of cpu flags on ARM is 32 */
+# define MAX_CPU_FLAGS 32
+#endif
+
+VIR_LOG_INIT("cpu.cpu_arm");
+
 static const virArch archs[] = {
 VIR_ARCH_ARMV6L,
 VIR_ARCH_ARMV7B,
@@ -491,12 +504,203 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
 return 0;
 }
 
+#if defined(__aarch64__)
+/**
+ * virCPUarmCpuDataFromRegs:
+ *
+ * @data: 64-bit arm CPU specific data
+ *
+ * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU
+ * flags from AT_HWCAP. There are currently 32 valid flags  on ARM arch
+ * represented by each bit.
+ */
+static int
+virCPUarmCpuDataFromRegs(virCPUarmData *data)
+{
+/* Generate human readable flag list according to the order of */
+/* AT_HWCAP bit map */
+const char *flag_list[MAX_CPU_FLAGS] = {
+"fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",
+"crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",
+"jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",
+"asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",
+"ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};
+unsigned long cpuid, hwcaps;
+char **features = NULL;
+g_autofree char *cpu_feature_str = NULL;
+int cpu_feature_index = 0;
+size_t i;
+
+if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("CPUID registers unavailable"));
+return -1;
+}
+
+/* read the cpuid data from MIDR_EL1 register */
+asm("mrs %0, MIDR_EL1" : "=r" (cpuid));
+VIR_DEBUG("CPUID read from register:  0x%016lx", cpuid);
+
+/* parse the coresponding part_id bits */
+data->pvr = cpuid>>4&0xFFF;
+/* parse the coresponding vendor_id bits */
+data->vendor_id = cpuid>>24&0xFF;
+
+hwcaps = getauxval(AT_HWCAP);
+VIR_DEBUG("CPU flags read from register:  0x%016lx", hwcaps);
+
+if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)
+return -1;
+
+/* shift bit map mask to parse for CPU flags */
+for (i = 0; i< MAX_CPU_FLAGS; i++) {
+if (hwcaps & BIT_SHIFTS(i)) {
+features[cpu_feature_index] = g_strdup(flag_list[i]);
+cpu_feature_index++;
+}
+}
+
+if (cpu_feature_index > 1) {
+cpu_feature_str = virStringListJoin((const char **)features, " ");
+if (!cpu_feature_str)
+goto error;
+}
+data->features = g_strdup(cpu_feature_str);
+
+return 0;
+
+ error:
+virStringListFree(features);
+return -1;
+}
+
+static int
+virCPUarmDataParseFeatures(virCPUDefPtr cpu,
+   const virCPUarmData *cpuData)
+{
+int ret = -1;
+size_t i;
+char **features;
+
+if (!cpu || !cpuData)
+return ret;
+
+if (!(features = virStringSplitCount(cpuData->features, " ",
+ 0, &cpu->nfeatures)))
+return ret;
+if (cpu->nfeatures) {
+if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)
+goto error;
+
+for (i = 0; i < cpu->nfeatures; i++) {
+cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
+cpu->features[i].name = g_strdup(features[i]);
+}
+}
+
+ret = 0;
+
+ cleanup:
+virStringListFree(features);
+return ret;
+
+ error:
+for (i = 0; i < cpu->nfeatures; i++)
+VIR_FREE(cpu->features[i].name);
+VIR_FREE(cpu->features);
+cpu->nfeatures = 0;
+goto cleanup;
+}
+
+static int
+virCPUarmDecode(virCPUDefPtr cpu,
+const virCPUarmData *cpuData,
+virDomainCapsCPUModelsPtr models)
+{
+virCPUarmMapPtr map;
+virCPUarmModelPtr model;
+virCPUarmVendorPtr vendor = NULL;
+
+if (!cpuData || !(map = virCPUarmGetMap()))
+return -1;
+
+if (!(model = virCPUarmModelFindByPVR(map, cpuData->pvr))) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("Cannot find CPU model with PVR 0x%03lx"),
+   cpuData->pvr);
+return -1;
+}
+
+if (!virCPUModelIsAllowed(model->name, models)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("CPU model %s is not supported by hypervisor"),
+   model->n

[PATCH V3 1/5] cpu_map: Introduce ARM cpu models

[ZhengZhenyu]

Introduce vendors and some commonly used models
for ARM arch, these will be used for virConnectionGetCapabilities
for ARM CPUs.

Signed-off-by: Zhenyu Zheng 
---
 src/cpu_map/Makefile.inc.am   |  7 +++
 src/cpu_map/arm_Falkor.xml| 16 
 src/cpu_map/arm_Kunpeng-920.xml   | 24 
 src/cpu_map/arm_ThunderX299xx.xml | 16 
 src/cpu_map/arm_cortex-a53.xml| 16 
 src/cpu_map/arm_cortex-a57.xml| 15 +++
 src/cpu_map/arm_cortex-a72.xml| 15 +++
 src/cpu_map/arm_vendors.xml   | 14 ++
 src/cpu_map/index.xml | 15 +++
 9 files changed, 138 insertions(+)
 create mode 100644 src/cpu_map/arm_Falkor.xml
 create mode 100644 src/cpu_map/arm_Kunpeng-920.xml
 create mode 100644 src/cpu_map/arm_ThunderX299xx.xml
 create mode 100644 src/cpu_map/arm_cortex-a53.xml
 create mode 100644 src/cpu_map/arm_cortex-a57.xml
 create mode 100644 src/cpu_map/arm_cortex-a72.xml
 create mode 100644 src/cpu_map/arm_vendors.xml

diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
index be64c9a0d4..93c2b19ddf 100644
--- a/src/cpu_map/Makefile.inc.am
+++ b/src/cpu_map/Makefile.inc.am
@@ -2,7 +2,14 @@
 
 cpumapdir = $(pkgdatadir)/cpu_map
 cpumap_DATA = \
+cpu_map/arm_cortex-a53.xml \
+   cpu_map/arm_cortex-a57.xml \
+   cpu_map/arm_cortex-a72.xml \
cpu_map/arm_features.xml \
+   cpu_map/arm_Kunpeng-920.xml \
+   cpu_map/arm_ThunderX299xx.xml \
+   cpu_map/arm_Falkor.xml \
+   cpu_map/arm_vendors.xml \
cpu_map/index.xml \
cpu_map/ppc64_vendors.xml \
cpu_map/ppc64_POWER7.xml \
diff --git a/src/cpu_map/arm_Falkor.xml b/src/cpu_map/arm_Falkor.xml
new file mode 100644
index 00..902ed2b6ba
--- /dev/null
+++ b/src/cpu_map/arm_Falkor.xml
@@ -0,0 +1,16 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_Kunpeng-920.xml b/src/cpu_map/arm_Kunpeng-920.xml
new file mode 100644
index 00..668b8b50dc
--- /dev/null
+++ b/src/cpu_map/arm_Kunpeng-920.xml
@@ -0,0 +1,24 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_ThunderX299xx.xml 
b/src/cpu_map/arm_ThunderX299xx.xml
new file mode 100644
index 00..9ab3d939e9
--- /dev/null
+++ b/src/cpu_map/arm_ThunderX299xx.xml
@@ -0,0 +1,16 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_cortex-a53.xml b/src/cpu_map/arm_cortex-a53.xml
new file mode 100644
index 00..963d6d36e3
--- /dev/null
+++ b/src/cpu_map/arm_cortex-a53.xml
@@ -0,0 +1,16 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_cortex-a57.xml b/src/cpu_map/arm_cortex-a57.xml
new file mode 100644
index 00..92a044ea92
--- /dev/null
+++ b/src/cpu_map/arm_cortex-a57.xml
@@ -0,0 +1,15 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_cortex-a72.xml b/src/cpu_map/arm_cortex-a72.xml
new file mode 100644
index 00..9664eacd7b
--- /dev/null
+++ b/src/cpu_map/arm_cortex-a72.xml
@@ -0,0 +1,15 @@
+
+  
+
+
+
+
+
+
+
+
+
+
+
+  
+
diff --git a/src/cpu_map/arm_vendors.xml b/src/cpu_map/arm_vendors.xml
new file mode 100644
index 00..703c2112b1
--- /dev/null
+++ b/src/cpu_map/arm_vendors.xml
@@ -0,0 +1,14 @@
+
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+
diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml
index 50b030de29..20646a031c 100644
--- a/src/cpu_map/index.xml
+++ b/src/cpu_map/index.xml
@@ -85,6 +85,21 @@
   
 
   
+
 
+
+
+
+
+
+
+
+
+
+
+
+
+
+
   
 
-- 
2.26.0.windows.1

[PATCH V3 4/5] cpu: Add helper funtions to parse vendor and model

[ZhengZhenyu]

Add helper functions to parse vendor and model from
xml for ARM arch, and use them as callbacks when
load cpu maps.

Signed-off-by: Zhenyu Zheng 
---
 src/cpu/cpu_arm.c | 173 +-
 1 file changed, 170 insertions(+), 3 deletions(-)

diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index 2009904cc9..6e9ff9bf11 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -204,6 +204,174 @@ virCPUarmMapFeatureParse(xmlXPathContextPtr ctxt 
G_GNUC_UNUSED,
 return 0;
 }
 
+static virCPUarmVendorPtr
+virCPUarmVendorFindByID(virCPUarmMapPtr map,
+unsigned long vendor_id)
+{
+size_t i;
+
+for (i = 0; i < map->nvendors; i++) {
+if (map->vendors[i]->value == vendor_id)
+return map->vendors[i];
+}
+
+return NULL;
+}
+
+
+static virCPUarmVendorPtr
+virCPUarmVendorFindByName(virCPUarmMapPtr map,
+  const char *name)
+{
+size_t i;
+
+for (i = 0; i < map->nvendors; i++) {
+if (STREQ(map->vendors[i]->name, name))
+return map->vendors[i];
+}
+
+return NULL;
+}
+
+
+static int
+virCPUarmVendorParse(xmlXPathContextPtr ctxt,
+   const char *name,
+   void *data)
+{
+virCPUarmMapPtr map = data;
+virCPUarmVendorPtr vendor = NULL;
+int ret = -1;
+
+if (VIR_ALLOC(vendor) < 0)
+return ret;
+
+vendor->name = g_strdup(name);
+
+if (virCPUarmVendorFindByName(map, vendor->name)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("CPU vendor %s already defined"),
+   vendor->name);
+goto cleanup;
+}
+
+if (virXPathULongHex("string(@value)", ctxt, &vendor->value) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Missing CPU vendor value"));
+goto cleanup;
+}
+
+if (virCPUarmVendorFindByID(map, vendor->value)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("CPU vendor value 0x%2lx already defined"),
+   vendor->value);
+goto cleanup;
+}
+
+if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virCPUarmVendorFree(vendor);
+return ret;
+
+}
+
+static virCPUarmModelPtr
+virCPUarmModelFind(virCPUarmMapPtr map,
+   const char *name)
+{
+size_t i;
+
+for (i = 0; i < map->nmodels; i++) {
+if (STREQ(map->models[i]->name, name))
+return map->models[i];
+}
+
+return NULL;
+}
+
+#if defined(__aarch64__)
+static virCPUarmModelPtr
+virCPUarmModelFindByPVR(virCPUarmMapPtr map,
+unsigned long pvr)
+{
+size_t i;
+
+for (i = 0; i < map->nmodels; i++) {
+if (map->models[i]->data.pvr == pvr)
+return map->models[i];
+}
+
+return NULL;
+}
+#endif
+
+static int
+virCPUarmModelParse(xmlXPathContextPtr ctxt,
+  const char *name,
+  void *data)
+{
+virCPUarmMapPtr map = data;
+virCPUarmModel *model;
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree char *vendor = NULL;
+
+if (VIR_ALLOC(model) < 0)
+goto error;
+
+model->name = g_strdup(name);
+
+if (virCPUarmModelFind(map, model->name)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("CPU model %s already defined"),
+   model->name);
+goto error;
+}
+
+if (virXPathBoolean("boolean(./vendor)", ctxt)) {
+vendor = virXPathString("string(./vendor/@name)", ctxt);
+if (!vendor) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Invalid vendor element in CPU model %s"),
+   model->name);
+goto error;
+}
+
+if (!(model->vendor = virCPUarmVendorFindByName(map, vendor))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown vendor %s referenced by CPU model %s"),
+   vendor, model->name);
+goto error;
+}
+}
+
+if (!virXPathBoolean("boolean(./pvr)", ctxt)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Missing PVR information for CPU model %s"),
+   model->name);
+goto error;
+}
+
+if (virXPathULongHex("string(./pvr/@value)", ctxt, &model->data.pvr) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Missing or invalid PVR value in CPU model %s"),
+   model->name);
+goto error;
+}
+
+if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
+goto error;
+
+return 0;
+
+ error:
+virCPUarmModelFree(model);
+return -1;
+}
+
 static virCPUarmMapPtr
 virCPUarmLoadMap(void)
 {
@@ -211,8 +379,8 @@ virCPUarmLoadMap(void)
 
 map = virCPUarmMapNew();
 
-if (cpuMapLoad("arm", NULL, virCPUa

[PATCH V3 0/5] Introduce getHost support for ARM CPU driver

[ZhengZhenyu]

Introduce getHost support for ARM CPU driver. First add
some data about commonly used ARM CPU models, and their
vendors into cpu_map, then added some helper methods as
callbacks to load them. Read and parse vendor_id, part_id
and CPU flags of local CPU from corresponding registers.

Signed-off-by: Zhenyu Zheng 

ZhengZhenyu (5):
  cpu_map: Introduce ARM cpu models
  cpu: Introduce virCPUarmData to virCPUData
  cpu: Introduce ARM related structs
  cpu: Add helper funtions to parse vendor and model
  cpu: Introduce getHost support for ARM CPU driver

 src/cpu/Makefile.inc.am   |   1 +
 src/cpu/cpu.h |   2 +
 src/cpu/cpu_arm.c | 450 +-
 src/cpu/cpu_arm_data.h|  31 ++
 src/cpu_map/Makefile.inc.am   |   7 +
 src/cpu_map/arm_Falkor.xml|  16 ++
 src/cpu_map/arm_Kunpeng-920.xml   |  24 ++
 src/cpu_map/arm_ThunderX299xx.xml |  16 ++
 src/cpu_map/arm_cortex-a53.xml|  16 ++
 src/cpu_map/arm_cortex-a57.xml|  15 +
 src/cpu_map/arm_cortex-a72.xml|  15 +
 src/cpu_map/arm_vendors.xml   |  14 +
 src/cpu_map/index.xml |  15 +
 13 files changed, 619 insertions(+), 3 deletions(-)
 create mode 100644 src/cpu/cpu_arm_data.h
 create mode 100644 src/cpu_map/arm_Falkor.xml
 create mode 100644 src/cpu_map/arm_Kunpeng-920.xml
 create mode 100644 src/cpu_map/arm_ThunderX299xx.xml
 create mode 100644 src/cpu_map/arm_cortex-a53.xml
 create mode 100644 src/cpu_map/arm_cortex-a57.xml
 create mode 100644 src/cpu_map/arm_cortex-a72.xml
 create mode 100644 src/cpu_map/arm_vendors.xml

-- 
2.26.0.windows.1