[libvirt] [PATCH] domain: avoid slash characters to the new domain name.

[Julio Faracco]

The 'domrename' command needs to check if the new domain name contains
the slash character. This character is not accepted by libvirt XML
definition because it is an invalid char (see Cole's commit b1fc6a7b7).
This commit enhace the 'domrename' command adding this check.

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

Signed-off-by: Julio Faracco 
---
 src/internal.h   | 9 +
 src/libvirt-domain.c | 1 +
 2 files changed, 10 insertions(+)

diff --git a/src/internal.h b/src/internal.h
index 1760e3b69..e976b1caa 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -484,6 +484,15 @@
 goto label; \
 } \
 } while (0)
+# define virCheckNonSlashGoto(argname, label) \
+do { \
+if (strchr(argname, '/')) { \
+   virReportInvalidArg(ctl, \
+   _("name %s cannot contain '/'"), \
+   argname); \
+goto label; \
+} \
+} while (0)
 
 
 
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 63d2ae23d..47bc59b11 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8536,6 +8536,7 @@ virDomainRename(virDomainPtr dom,
 virResetLastError();
 virCheckDomainReturn(dom, -1);
 virCheckNonEmptyStringArgGoto(new_name, error);
+virCheckNonSlashGoto(new_name, error);
 virCheckReadOnlyGoto(dom->conn->flags, error);
 
 if (dom->conn->driver->domainRename) {
-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu_monitor: query-cpu-model-baseline QMP command

[Collin Walling]

On 04/16/2018 02:06 AM, Chris Venteicher wrote:
>  Function qemuMonitorGetCPUModelBaseline exposed to carry out a QMP
>  query-cpu-model-baseline transaction with QEMU.
> 
>  QEMU determines a baseline CPU Model from two input CPU Models to
>  complete the query-cpu-model-baseline transaction.
> ---
>  src/qemu/qemu_monitor.c  | 16 +
>  src/qemu/qemu_monitor.h  |  5 
>  src/qemu/qemu_monitor_json.c | 56 
> 
>  src/qemu/qemu_monitor_json.h |  7 ++
>  4 files changed, 84 insertions(+)
> 
[...]
>  
>  int qemuMonitorJSONGetCommands(qemuMonitorPtr mon,
> char ***commands)
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 045df4919..6c33049c6 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -361,6 +361,13 @@ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> mon,
>  qemuMonitorCPUModelInfoPtr 
> *model_info)
>  ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5);
>  
> +int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
> +   qemuMonitorCPUModelInfoPtr model_a,
> +   qemuMonitorCPUModelInfoPtr model_b,
> +   qemuMonitorCPUModelInfoPtr 
> *model_baseline)
> +ATTRIBUTE_NONNULL(4);
> +
> +

I believe you want NONNULL(2) and (3) here as well.

>  int qemuMonitorJSONGetCommands(qemuMonitorPtr mon,
> char ***commands)
>  ATTRIBUTE_NONNULL(2);
> 


-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/3] query-cpu-model-baseline QMP command

[Collin Walling]

On 04/16/2018 02:06 AM, Chris Venteicher wrote:
> Implementation of libvirt functions to support the
> QEMU query-cpu-model-baseline QMP command.
> 
> This is part of resolution of: 
> https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> 
> Signed-off-by: Chris Venteicher 
> 
> Chris Venteicher (3):
>   qemu_monitor_json: Populate CPUModelInfo struct from json
>   qemu_monitor_json: Build Json CPU Model Info
>   qemu_monitor: query-cpu-model-baseline QMP command
> 
>  src/qemu/qemu_monitor.c  |  16 
>  src/qemu/qemu_monitor.h  |   5 ++
>  src/qemu/qemu_monitor_json.c | 179 
> +--
>  src/qemu/qemu_monitor_json.h |   7 ++
>  4 files changed, 185 insertions(+), 22 deletions(-)
> 
Hi Chris,

I'm working on a neighboring item with implementing cpu-comparison. Since there
are some similarities with both comparison and baseline, I'd like for us to
work together on driving both features forward.

Thus far, I've created the qemu_monitor_json interface, tied it into the qemu 
driver, and can issue virsh cpu-compare  successfully. (Though it's
no where near perfect yet ;) )

I've already noted some differences between our JSON interaces (namely, yours
works with qemuMonitorCPUModelInfoPtrs, and mine works with virCPUDefPtrs). 

Ultimately, I think your patches are in good shape.

Let's discuss further on my comparison patches posted here:

https://www.redhat.com/archives/libvir-list/2018-April/msg01375.html

I'm open to any suggestions / design ideas you have.

-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v1 3/4] qemu: implement query-cpu-model-comparison via qemu

[Collin Walling]

Interfaces with QEMU to compare two CPU models. The command takes
two CPU models, A and B, that are given a model name and an optional
list of CPU features. Through the query-cpu-model-comparison command
sent to QEMU via QMP, a third CPU model, C, is returned that contains
the comparison result (identical, superset, subset, incompatible) as
its model name as well as a list of properties (aka CPU features)
responsible for this result.

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_capabilities.c |  53 ++
 src/qemu/qemu_capabilities.h |   6 ++
 src/qemu/qemu_monitor.c  |  14 +
 src/qemu/qemu_monitor.h  |   6 ++
 src/qemu/qemu_monitor_json.c | 128 +++
 src/qemu/qemu_monitor_json.h |   6 ++
 6 files changed, 213 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d2eb813..d385ad8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "virtio-tablet-ccw",
   "qcow2-luks",
   "pcie-pci-bridge",
+  "query-cpu-model-comparison",
 );
 
 
@@ -1030,6 +1031,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
 { "query-hotpluggable-cpus", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS },
 { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA },
 { "query-cpu-model-expansion", QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION },
+{ "query-cpu-model-comparison", QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON },
 { "query-cpu-definitions", QEMU_CAPS_QUERY_CPU_DEFINITIONS },
 { "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES },
 };
@@ -4930,3 +4932,54 @@ virQEMUCapsSetMicrocodeVersion(virQEMUCapsPtr qemuCaps,
 {
 qemuCaps->microcodeVersion = microcodeVersion;
 }
+
+
+static virQEMUCapsInitQMPCommandPtr
+virQEMUCapsSetupBinary(char *binary)
+{
+virQEMUCapsInitQMPCommandPtr cmd;
+char *qmperr = NULL;
+
+if (!(cmd = virQEMUCapsInitQMPCommandNew(binary, "/tmp", -1, -1, )))
+goto cleanup;
+
+if (virQEMUCapsInitQMPCommandRun(cmd, false) != 0)
+goto cleanup;
+
+if (qemuMonitorSetCapabilities(cmd->mon) < 0) {
+VIR_DEBUG("Failed to set monitor capabilities %s",
+  virGetLastErrorMessage());
+goto cleanup;
+}
+
+return cmd;
+
+ cleanup:
+virQEMUCapsInitQMPCommandFree(cmd);
+return NULL;
+}
+
+
+qemuMonitorCPUModelInfoPtr
+virQEMUCapsProbeQMPCPUModelComparison(char *binary,
+  virCPUDefPtr cpuA,
+  virCPUDefPtr cpuB)
+{
+virQEMUCapsInitQMPCommandPtr cmd;
+qemuMonitorCPUModelInfoPtr cpuC = NULL;
+qemuMonitorCPUModelInfoPtr ret = NULL;
+
+if (!(cmd = virQEMUCapsSetupBinary(binary)))
+goto cleanup;
+
+if (qemuMonitorGetCPUModelComparison(cmd->mon, cpuA, cpuB, ) < 0)
+goto cleanup;
+
+ret = cpuC;
+cpuC = NULL;
+
+ cleanup:
+virQEMUCapsInitQMPCommandFree(cmd);
+qemuMonitorCPUModelInfoFree(cpuC);
+return ret;
+}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index a959931..f27a359 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -452,6 +452,7 @@ typedef enum {
 QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */
 QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */
 QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */
+QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON, /* qmp query-cpu-model-comparison */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
@@ -578,4 +579,9 @@ bool virQEMUCapsGuestIsNative(virArch host,
 bool virQEMUCapsCPUFilterFeatures(const char *name,
   void *opaque);
 
+qemuMonitorCPUModelInfoPtr
+virQEMUCapsProbeQMPCPUModelComparison(char *binary,
+  virCPUDefPtr cpuA,
+  virCPUDefPtr cpuB);
+
 #endif /* __QEMU_CAPABILITIES_H__*/
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 22f0522..1981b62 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3792,6 +3792,20 @@ qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr 
model_info)
 }
 
 
+int
+qemuMonitorGetCPUModelComparison(qemuMonitorPtr mon,
+ virCPUDefPtr cpuA,
+ virCPUDefPtr cpuB,
+ qemuMonitorCPUModelInfoPtr *cpuC)
+{
+VIR_DEBUG("cpuA=%p cpuB=%p", cpuA, cpuB);
+
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONGetCPUModelComparison(mon, cpuA, cpuB, cpuC);
+}
+
+
 qemuMonitorCPUModelInfoPtr
 qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig)
 {
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 9556a51..cc30184 100644
--- a/src/qemu/qemu_monitor.h
+++ 

[libvirt] [PATCH v1 0/4] CPU Model Comparsion via QEMU

[Collin Walling]

[Overview]

This patch series implements an interface to "query-cpu-model-comparison"
(available QEMU ~2.8.0) via virsh cpu-compare.

[Using This Feature]

Run virsh cpu-compare (ensure you are running the virsh in your build dir) and
pass it an xml file describing a cpu definition. You can copy the cpu xml from
virsh capabilities (if you want to compare the host cpu to itself), or a cpu 
defined in any guest xml. Additionally, you can create a cpu xml as such (e.g.
for s390x):


s390x
model_name



NOTE: the presence of  is optional and it will treat the cpu defined in 
  the xml as a host cpu. This will disregard all feature policies (i.e. 
  all features listed will behave with policy='require', even if disable 
  is specified).

NOTE: as s390x only supports feature policies 'require' and 'disable', I am
  uncertain how to handle the other policies when parsing CPUDef to JSON.

[Example Output]

On an s390x system running a z13.2, this is the expected output (where each file
describes a CPU model corresponding to the name of the file):

$ virsh cpu-compare zEC12.xml
Host CPU is a superset of CPU described in zEC12.xml

$ virsh cpu-compare z13.2.xml
CPU described in z13.2.xml is identical to host CPU

$ virsh cpu-compare z14.xml
CPU described in z14.xml is incompatible with host CPU

$ virsh cpu-compare z14.xml --error
error: Failed to compare host CPU with z14.xml
error: the CPU is incompatible with host CPU

$ virsh cpu-compare z12345.xml 
error: Failed to compare host CPU with z12345cpu.xml
error: internal error: unable to execute QEMU command 
'query-cpu-model-comparison': The CPU definition 'z12345-base' is unknown.

[Patch Rundown]

The first patch copies the host CPU definition from qemuCaps to virCaps so
we can easily access the host CPU model and features when we handle the CPU
comparison in qemu_driver. Note that we take care to not clobber anything
already stored in the host CPU definition until we have successfully 
constructed a new host CPU definition.

The second patch refactors the xml -> CPUDef code. This is used in 
virCPUCompareXML, qemuCompareCPU (and potentially for baseline).

The third patch creates the interface from libvirt to QEMU and handles parsing
the CPUDef -> JSON and JSON -> CPUModelInfo. In order to respect the data
returned from this command, we capture the "responsible_properties" field
from the comparison command even though it is not entirely necessary for what
we are trying to accomplish here (which is to simply report if the comparison
result is "incompatible", "superset", or "identical").

The fourth patch creates the interface from virsh cpu-compare to the qemu 
driver.
The qemu driver will handle parsing the xml passed to the virsh command and
parsing the result of the QMP command. Note that the "incompatible" and 
"subset" results are grouped together (and report "incompatible").

TODO:
- implement cpu-baseline (I've noted Chris Venteicher's current progress on 
this):
https://www.redhat.com/archives/libvir-list/2018-April/msg01274.html
- update qemucapabilitiestest files for qemu >= 2.8.0 (need to add 
comparison command)
- consider adding a new test file for comparing cpu models via QEMU if 
necessary

Collin Walling (4):
  qemu: place qemuCaps host cpu model in virCaps
  cpu_conf: xml to cpu definition parse helper
  qemu: implement query-cpu-model-comparison via qemu
  qemu: hook up cpu-comparison to qemu driver

 src/conf/cpu_conf.c  |  30 ++
 src/conf/cpu_conf.h  |   6 ++
 src/cpu/cpu.c|  14 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_capabilities.c | 108 +++-
 src/qemu/qemu_capabilities.h |   9 +++
 src/qemu/qemu_driver.c   |  96 
 src/qemu/qemu_monitor.c  |  14 +
 src/qemu/qemu_monitor.h  |   6 ++
 src/qemu/qemu_monitor_json.c | 128 +++
 src/qemu/qemu_monitor_json.h |   6 ++
 11 files changed, 404 insertions(+), 14 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v1 4/4] qemu: hook up cpu-comparison to qemu driver

[Collin Walling]

The virsh cpu-compare command accepts an xml file that describes
a cpu definition and compares it to a master xml file containing
the host CPU. Not all architectures follow this procedure, and
instead compare CPU's via QEMU. Let's hook up this capability
to the QEMU driver and, if the capabilitiy is available, compare
the host CPU with the CPU defined in the xml file.

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_capabilities.c |  2 +-
 src/qemu/qemu_capabilities.h |  3 ++
 src/qemu/qemu_driver.c   | 96 
 3 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d385ad8..e4ce086 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -657,7 +657,7 @@ virQEMUCapsFindBinary(const char *format,
 return ret;
 }
 
-static char *
+char *
 virQEMUCapsFindBinaryForArch(virArch hostarch,
  virArch guestarch)
 {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f27a359..01770f9 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -579,6 +579,9 @@ bool virQEMUCapsGuestIsNative(virArch host,
 bool virQEMUCapsCPUFilterFeatures(const char *name,
   void *opaque);
 
+char *virQEMUCapsFindBinaryForArch(virArch hostarch,
+   virArch guestarch);
+
 qemuMonitorCPUModelInfoPtr
 virQEMUCapsProbeQMPCPUModelComparison(char *binary,
   virCPUDefPtr cpuA,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 92f5fe6..c87792b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13100,6 +13100,85 @@ qemuNodeDeviceReset(virNodeDevicePtr dev)
 return ret;
 }
 
+
+/**
+ * qemuCompareCPU:
+ * @binary: QEMU binary to issue the comparison command to
+ * @host: host CPU definition
+ * @xml: XML description of either guest or host CPU to be compared with @host
+ * @failIncompatible: return an error instead of VIR_CPU_COMPARE_INCOMPATIBLE
+ *
+ * Compares the target CPU described by @xml with @host CPU to produce a third
+ * model containing the comparison result and the list of features responsible
+ * format the result. This function discards the responsible features.
+ *
+ * Compared model results:
+ *
+ * "incompatible": target cpu cannot run on @host.
+ *
+ * "subset":@host is an older cpu model than target, or @host does not
+ *  support all features enabled on target.
+ *
+ *  This result is considered incompatible.
+ *
+ * "identical": @host and target are identical; target can run on @host.
+ *
+ * "superset":  @host is a newer cpu model than target, or @host supports some
+ *  features not supported by target; target can run on @host.
+ *
+ * Returns: virCPUCompareResult based on the produced "compared" model's name,
+ *  or VIR_CPU_COMPARE_ERROR upon error.
+ */
+static virCPUCompareResult
+qemuCompareCPU(char *binary,
+   virCPUDefPtr hostCPU,
+   const char *xml,
+   bool failIncompatible)
+{
+virCPUDefPtr targetCPU = NULL;
+qemuMonitorCPUModelInfoPtr result = NULL;
+virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
+
+VIR_DEBUG("binary=%s, hostCPU=%p, xml=%s", binary, hostCPU, NULLSTR(xml));
+
+if (!hostCPU || !hostCPU->model) {
+if (failIncompatible) {
+virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s",
+   _("cannot get host CPU capabilities"));
+} else {
+VIR_WARN("cannot get host CPU capabilities");
+ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+}
+goto cleanup;
+}
+
+if (virCPUDefParseXMLHelper(xml, NULL, VIR_CPU_TYPE_AUTO, ) < 0)
+goto cleanup;
+
+if (!(result = virQEMUCapsProbeQMPCPUModelComparison(binary, hostCPU,
+ targetCPU)))
+goto cleanup;
+
+if (STREQ(result->name, "incompatible") ||
+STREQ(result->name, "subset"))
+ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+else if (STREQ(result->name, "identical"))
+ret = VIR_CPU_COMPARE_IDENTICAL;
+else if (STREQ(result->name, "superset"))
+ret = VIR_CPU_COMPARE_SUPERSET;
+
+if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {
+ret = VIR_CPU_COMPARE_ERROR;
+virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);
+}
+
+ cleanup:
+virCPUDefFree(targetCPU);
+qemuMonitorCPUModelInfoFree(result);
+return ret;
+}
+
+
 static int
 qemuConnectCompareCPU(virConnectPtr conn,
   const char *xmlDesc,
@@ -13109,6 +13188,8 @@ qemuConnectCompareCPU(virConnectPtr conn,
 int ret = VIR_CPU_COMPARE_ERROR;
 virCapsPtr caps = NULL;
 bool failIncompatible;
+char *binary = NULL;
+virQEMUCapsPtr qemuCaps = NULL;
 
 

[libvirt] [PATCH v1 1/4] qemu: place qemuCaps host cpu model in virCaps

[Collin Walling]

Some architectures retrieve their host CPU model data from QEMU
via qmp-query-cpu-model-expansion and stores it in the QEMU cache
files. Let's take this data and also store it in virCaps so we
can easily retrieve the host CPU model for later use.

We should also take care to ensure that we always query the same
cache file consistently between libvirtd executions, so let's
query the qemuCaps from the first qemu-system-ARCH found in $PATH
(via virQEMUCapsCacheLookup).

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_capabilities.c | 53 
 1 file changed, 53 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index cb716ff..d2eb813 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -896,6 +896,54 @@ virQEMUCapsProbeHostCPUForEmulator(virArch hostArch,
 }
 
 
+static int
+virQEMUProbeHostCPUModelFromBinary(virFileCachePtr cache,
+   virArch hostarch,
+   virCPUDefPtr *hostCPU)
+{
+char *binary;
+virQEMUCapsPtr qemuCaps = NULL;
+virCPUDefPtr qemuCpu;
+virCPUDefPtr tmp = NULL;
+int ret = -1;
+
+if (!(binary = virQEMUCapsFindBinaryForArch(hostarch, hostarch)))
+goto cleanup;
+
+if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary)))
+goto cleanup;
+
+/* If QEMU does not report the host's cpu model, then fail gracefully */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) {
+ret = 0;
+goto cleanup;
+}
+
+if (!(qemuCpu = virQEMUCapsGetHostModel(qemuCaps, VIR_DOMAIN_VIRT_KVM,
+VIR_QEMU_CAPS_HOST_CPU_REPORTED)))
+goto cleanup;
+
+if (VIR_ALLOC(tmp) < 0)
+goto cleanup;
+
+if (!(tmp = virCPUDefCopyWithoutModel(*hostCPU)))
+goto cleanup;
+
+if (virCPUDefCopyModel(tmp, qemuCpu, false) < 0)
+goto cleanup;
+
+virCPUDefFree(*hostCPU);
+VIR_STEAL_PTR(*hostCPU, tmp);
+ret = 0;
+
+ cleanup:
+VIR_FREE(binary);
+virCPUDefFree(tmp);
+virObjectUnref(qemuCaps);
+return ret;
+}
+
+
 virCapsPtr
 virQEMUCapsInit(virFileCachePtr cache)
 {
@@ -922,6 +970,11 @@ virQEMUCapsInit(virFileCachePtr cache)
 if (!(caps->host.cpu = virCPUProbeHost(caps->host.arch)))
 VIR_WARN("Failed to get host CPU");
 
+/* Some archs get host cpu information via QEMU */
+if (caps->host.cpu && !caps->host.cpu->model &&
+virQEMUProbeHostCPUModelFromBinary(cache, hostarch, >host.cpu) < 
0)
+VIR_WARN("Failed to get host CPU model from QEMU");
+
 /* Add the power management features of the host */
 if (virNodeSuspendGetTargetMask(>host.powerMgmt) < 0)
 VIR_WARN("Failed to get host power management capabilities");
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v1 2/4] cpu_conf: xml to cpu definition parse helper

[Collin Walling]

Implement an xml to virCPUDefPtr helper that handles
the ctxt prerequisite for virCPUDefParseXML.

Signed-off-by: Collin Walling 
---
 src/conf/cpu_conf.c  | 30 ++
 src/conf/cpu_conf.h  |  6 ++
 src/cpu/cpu.c| 14 +-
 src/libvirt_private.syms |  1 +
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 43a3ab5..b2e726a 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -250,6 +250,36 @@ virCPUDefCopy(const virCPUDef *cpu)
 }
 
 
+int
+virCPUDefParseXMLHelper(const char *xml,
+const char *xpath,
+virCPUType type,
+virCPUDefPtr *cpu)
+{
+xmlDocPtr doc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ret = -1;
+
+if (!xml) {
+virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing CPU definition"));
+goto cleanup;
+}
+
+if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), )))
+goto cleanup;
+
+if (virCPUDefParseXML(ctxt, xpath, type, cpu) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+xmlFreeDoc(doc);
+xmlXPathFreeContext(ctxt);
+return ret;
+}
+
+
 /*
  * Parses CPU definition XML from a node pointed to by @xpath. If @xpath is
  * NULL, the current node of @ctxt is used (i.e., it is a shortcut to ".").
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index 9f2e7ee..f8f57fa 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -184,6 +184,12 @@ virCPUDefPtr
 virCPUDefCopyWithoutModel(const virCPUDef *cpu);
 
 int
+virCPUDefParseXMLHelper(const char *xml,
+const char *xpath,
+virCPUType type,
+virCPUDefPtr *cpu);
+
+int
 virCPUDefParseXML(xmlXPathContextPtr ctxt,
   const char *xpath,
   virCPUType mode,
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 047e3b1..8b829a3 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -114,31 +114,19 @@ virCPUCompareXML(virArch arch,
  const char *xml,
  bool failIncompatible)
 {
-xmlDocPtr doc = NULL;
-xmlXPathContextPtr ctxt = NULL;
 virCPUDefPtr cpu = NULL;
 virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
 
 VIR_DEBUG("arch=%s, host=%p, xml=%s",
   virArchToString(arch), host, NULLSTR(xml));
 
-if (!xml) {
-virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing CPU definition"));
-goto cleanup;
-}
-
-if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), )))
-goto cleanup;
-
-if (virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, ) < 0)
+if (virCPUDefParseXMLHelper(xml, NULL, VIR_CPU_TYPE_AUTO, ) < 0)
 goto cleanup;
 
 ret = virCPUCompare(arch, host, cpu, failIncompatible);
 
  cleanup:
 virCPUDefFree(cpu);
-xmlXPathFreeContext(ctxt);
-xmlFreeDoc(doc);
 
 return ret;
 }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cab324c..502fe48 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -86,6 +86,7 @@ virCPUDefIsEqual;
 virCPUDefListFree;
 virCPUDefListParse;
 virCPUDefParseXML;
+virCPUDefParseXMLHelper;
 virCPUDefStealModel;
 virCPUDefUpdateFeature;
 virCPUModeTypeToString;
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] lxc: s/subtreee/subtree/

[Ján Tomko]

On Sun, Apr 15, 2018 at 04:30:10PM +0100, Radostin Stoyanov wrote:

Signed-off-by: Radostin Stoyanov 
---
src/lxc/lxc_container.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

And pushed.

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 73/73] qemuxml2xmltest: Add status XML tests for migration params

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:42:03PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
.../migration-in-params-in.xml| 400 +
.../migration-in-params-out.xml   |   1 +
.../migration-out-params-in.xml   | 414 ++
.../migration-out-params-out.xml  |   1 +
tests/qemuxml2xmltest.c   |   2 +
5 files changed, 818 insertions(+)
create mode 100644 tests/qemustatusxml2xmldata/migration-in-params-in.xml
create mode 12 tests/qemustatusxml2xmldata/migration-in-params-out.xml
create mode 100644 tests/qemustatusxml2xmldata/migration-out-params-in.xml
create mode 12 tests/qemustatusxml2xmldata/migration-out-params-out.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 71/73] qemu: Store API flags for async jobs in status XML

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:42:01PM +0200, Jiri Denemark wrote:

This will help us decide what to do when libvirtd is restarted while an
async job is running.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c|   8 +
.../migration-out-nbd-out.xml | 450 +-
2 files changed, 457 insertions(+), 1 deletion(-)
mode change 12 => 100644 
tests/qemustatusxml2xmldata/migration-out-nbd-out.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 72/73] qemu: Don't delete TLS objects unless TLS migration was requested

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:42:02PM +0200, Jiri Denemark wrote:

Trying to delete the non-existent TLS objects results in ugly error
messages in the log, which could easily confuse users. Let's avoid this
confusion by not trying to delete the objects if we were not asked to
enable TLS migration and thus we didn't created the objects anyway.

This patch restores the behavior to the state before "qemu: Reset all
migration parameters".


Finally. The suspense was killing me.



Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 12 ++--
src/qemu/qemu_migration_params.c | 18 --
src/qemu/qemu_migration_params.h |  3 ++-
src/qemu/qemu_process.c  |  6 --
4 files changed, 24 insertions(+), 15 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 70/73] qemu: Drop priv->job.postcopyEnabled bool

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:42:00PM +0200, Jiri Denemark wrote:

We store the flags passed to the API which started the migration. Let's
use them instead of a separate bool to check if post-copy migration was
requested.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c| 1 -
src/qemu/qemu_domain.h| 1 -
src/qemu/qemu_driver.c| 2 +-
src/qemu/qemu_migration.c | 4 
4 files changed, 1 insertion(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 69/73] qemu: Drop priv->job.dump_memory_only bool

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:59PM +0200, Jiri Denemark wrote:

We store the flags passed to the API which started QEMU_ASYNC_JOB_DUMP
and we can use them to check whether a memory-only dump is running.
There's no need for a specific bool flag.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c | 1 -
src/qemu/qemu_domain.h | 1 -
src/qemu/qemu_driver.c | 4 +---
3 files changed, 1 insertion(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 68/73] qemu: Properly avoid cancelling memory-only dump

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:58PM +0200, Jiri Denemark wrote:

migrate_cancel QMP command cannot be used for cancelling memory-only
dumps and priv->job.dump_memory_only is used for reporting an error if
someone calls virDomainAbortJob when memory-only dump job is running.

Since commit 150930e3098 the dump_memory_only flag is set only
dump-guest-memory command was called without the detach parameter. This


s/only/only if/


would incorrectly allow libvirt to send migrate_cancel while the
detached memory-only dump is running.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 67/73] qemu: Store API flags for async jobs in qemuDomainJobObj

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:57PM +0200, Jiri Denemark wrote:

When an async job is running, we sometimes need to know how it was
started to distinguish between several types of the job, e.g., post-copy
vs. normal migration. So far we added a specific bool item to
qemuDomainJobObj for such cases, which doesn't scale very well and
storing such bools in status XML would be painful so we didn't do it.

A better approach is to store the flags passed to the API which started
the async job, which can be easily stored in status XML.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c| 10 --
src/qemu/qemu_domain.h|  4 +++-
src/qemu/qemu_driver.c| 31 +++
src/qemu/qemu_migration.c | 20 +---
src/qemu/qemu_process.c   |  5 +++--
src/qemu/qemu_process.h   |  3 ++-
6 files changed, 48 insertions(+), 25 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH 1/4] guests: Add libvirt-glib+mingw project

[Andrea Bolognani]

Just like libvirt+mingw, we want to keep the native toolchain
and the MinGW one separate.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-fedora-rawhide/main.yml  | 1 +
 guests/vars/projects/{libvirt-glib.yml => libvirt-glib+mingw.yml} | 2 --
 guests/vars/projects/libvirt-glib.yml | 4 
 3 files changed, 1 insertion(+), 6 deletions(-)
 copy guests/vars/projects/{libvirt-glib.yml => libvirt-glib+mingw.yml} (87%)

diff --git a/guests/host_vars/libvirt-fedora-rawhide/main.yml 
b/guests/host_vars/libvirt-fedora-rawhide/main.yml
index c905fb0..9544ffe 100644
--- a/guests/host_vars/libvirt-fedora-rawhide/main.yml
+++ b/guests/host_vars/libvirt-fedora-rawhide/main.yml
@@ -9,6 +9,7 @@ projects:
   - libvirt-cim
   - libvirt-dbus
   - libvirt-glib
+  - libvirt-glib+mingw
   - libvirt-go
   - libvirt-go-xml
   - libvirt-perl
diff --git a/guests/vars/projects/libvirt-glib.yml 
b/guests/vars/projects/libvirt-glib+mingw.yml
similarity index 87%
copy from guests/vars/projects/libvirt-glib.yml
copy to guests/vars/projects/libvirt-glib+mingw.yml
index abccb60..dec8e5c 100644
--- a/guests/vars/projects/libvirt-glib.yml
+++ b/guests/vars/projects/libvirt-glib+mingw.yml
@@ -4,9 +4,7 @@ packages:
   - gobject-introspection
   - gtk-doc
   - intltool
-  - libxml2
   - mingw32-glib2
   - mingw64-glib2
   - mingw32-libxml2
   - mingw64-libxml2
-  - vala
diff --git a/guests/vars/projects/libvirt-glib.yml 
b/guests/vars/projects/libvirt-glib.yml
index abccb60..13a5128 100644
--- a/guests/vars/projects/libvirt-glib.yml
+++ b/guests/vars/projects/libvirt-glib.yml
@@ -5,8 +5,4 @@ packages:
   - gtk-doc
   - intltool
   - libxml2
-  - mingw32-glib2
-  - mingw64-glib2
-  - mingw32-libxml2
-  - mingw64-libxml2
   - vala
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH 4/4] guests: Rename gtk-vnc mapping to gtk-vnc2

[Andrea Bolognani]

Both gtk-vnc and gtk-vnc2 exist: the former is based on GTK+
2 and is considered deprecated, the latter uses GTK+ 3 and
it's actively developed.

We need the GTK+ 3-based version for our builds: make that
more explicit by renaming the mapping. The MinGW variant of
the mapping already does this.

Signed-off-by: Andrea Bolognani 
---
 guests/vars/mappings.yml | 2 +-
 guests/vars/projects/virt-viewer.yml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index 1c1a872..f1777fe 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -164,7 +164,7 @@ mappings:
 default: gtk-doc
 deb: gtk-doc-tools
 
-  gtk-vnc:
+  gtk-vnc2:
 deb: libgtk-vnc-2.0-dev
 pkg: gtk-vnc
 rpm: gtk-vnc2-devel
diff --git a/guests/vars/projects/virt-viewer.yml 
b/guests/vars/projects/virt-viewer.yml
index 6f3dbf9..cd32176 100644
--- a/guests/vars/projects/virt-viewer.yml
+++ b/guests/vars/projects/virt-viewer.yml
@@ -1,7 +1,7 @@
 ---
 packages:
   - glib2
-  - gtk-vnc
+  - gtk-vnc2
   - gtk3
   - intltool
   - libgovirt
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH 2/4] guests: Add virt-viewer+mingw project

[Andrea Bolognani]

Just like libvirt+mingw and libvirt-glib+mingw, we want to
keep the native toolchain and the MinGW one separate.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-fedora-rawhide/main.yml   |  1 +
 .../{virt-viewer.yml => virt-viewer+mingw.yml} |  6 --
 guests/vars/projects/virt-viewer.yml   | 22 --
 3 files changed, 1 insertion(+), 28 deletions(-)
 copy guests/vars/projects/{virt-viewer.yml => virt-viewer+mingw.yml} (88%)

diff --git a/guests/host_vars/libvirt-fedora-rawhide/main.yml 
b/guests/host_vars/libvirt-fedora-rawhide/main.yml
index 9544ffe..43555d0 100644
--- a/guests/host_vars/libvirt-fedora-rawhide/main.yml
+++ b/guests/host_vars/libvirt-fedora-rawhide/main.yml
@@ -20,3 +20,4 @@ projects:
   - osinfo-db-tools
   - virt-manager
   - virt-viewer
+  - virt-viewer+mingw
diff --git a/guests/vars/projects/virt-viewer.yml 
b/guests/vars/projects/virt-viewer+mingw.yml
similarity index 88%
copy from guests/vars/projects/virt-viewer.yml
copy to guests/vars/projects/virt-viewer+mingw.yml
index f26ebff..f14de5b 100644
--- a/guests/vars/projects/virt-viewer.yml
+++ b/guests/vars/projects/virt-viewer+mingw.yml
@@ -1,11 +1,7 @@
 ---
 packages:
   - glib2
-  - gtk-vnc
-  - gtk3
   - intltool
-  - libgovirt
-  - libxml2
   - mingw32-glib2
   - mingw32-glib-networking
   - mingw32-gstreamer1-plugins-bad-free
@@ -28,5 +24,3 @@ packages:
   - mingw64-rest
   - mingw64-spice-gtk3
   - mingw64-usbredir
-  - spice-gtk3
-  - xmllint
diff --git a/guests/vars/projects/virt-viewer.yml 
b/guests/vars/projects/virt-viewer.yml
index f26ebff..6f3dbf9 100644
--- a/guests/vars/projects/virt-viewer.yml
+++ b/guests/vars/projects/virt-viewer.yml
@@ -6,27 +6,5 @@ packages:
   - intltool
   - libgovirt
   - libxml2
-  - mingw32-glib2
-  - mingw32-glib-networking
-  - mingw32-gstreamer1-plugins-bad-free
-  - mingw32-gstreamer1-plugins-good
-  - mingw32-gtk3
-  - mingw32-gtk-vnc2
-  - mingw32-libgovirt
-  - mingw32-libusbx
-  - mingw32-rest
-  - mingw32-spice-gtk3
-  - mingw32-usbredir
-  - mingw64-glib2
-  - mingw64-glib-networking
-  - mingw64-gstreamer1-plugins-bad-free
-  - mingw64-gstreamer1-plugins-good
-  - mingw64-gtk3
-  - mingw64-gtk-vnc2
-  - mingw64-libgovirt
-  - mingw64-libusbx
-  - mingw64-rest
-  - mingw64-spice-gtk3
-  - mingw64-usbredir
   - spice-gtk3
   - xmllint
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH 0/4] MinGW builds tweaks and fixes

[Andrea Bolognani]

Foreshadowed in

  https://www.redhat.com/archives/libvir-list/2018-April/msg01162.html

Andrea Bolognani (4):
  guests: Add libvirt-glib+mingw project
  guests: Add virt-viewer+mingw project
  guests: Add icoutils dependency to virt-viewer+mingw
  guests: Rename gtk-vnc mapping to gtk-vnc2

 guests/host_vars/libvirt-fedora-rawhide/main.yml   |  2 ++
 guests/vars/mappings.yml   |  6 +-
 .../{libvirt-glib.yml => libvirt-glib+mingw.yml}   |  2 --
 guests/vars/projects/libvirt-glib.yml  |  4 
 .../{virt-viewer.yml => virt-viewer+mingw.yml} |  7 +--
 guests/vars/projects/virt-viewer.yml   | 24 +-
 6 files changed, 9 insertions(+), 36 deletions(-)
 copy guests/vars/projects/{libvirt-glib.yml => libvirt-glib+mingw.yml} (87%)
 copy guests/vars/projects/{virt-viewer.yml => virt-viewer+mingw.yml} (88%)

-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH 3/4] guests: Add icoutils dependency to virt-viewer+mingw

[Andrea Bolognani]

The package is not needed for native builds, but MinGW builds
require it to be installed.

Signed-off-by: Andrea Bolognani 
---
 guests/vars/mappings.yml   | 4 
 guests/vars/projects/virt-viewer+mingw.yml | 1 +
 2 files changed, 5 insertions(+)

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index 1b88be0..1c1a872 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -173,6 +173,10 @@ mappings:
   hal:
 FreeBSD: hal
 
+  icoutils:
+default: icoutils
+CentOS6:
+
   intltool:
 default: intltool
 
diff --git a/guests/vars/projects/virt-viewer+mingw.yml 
b/guests/vars/projects/virt-viewer+mingw.yml
index f14de5b..be16f2c 100644
--- a/guests/vars/projects/virt-viewer+mingw.yml
+++ b/guests/vars/projects/virt-viewer+mingw.yml
@@ -1,6 +1,7 @@
 ---
 packages:
   - glib2
+  - icoutils
   - intltool
   - mingw32-glib2
   - mingw32-glib-networking
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/9] virobject: Introduce VIR_CLASS_NEW() macro

[Michal Privoznik]

On 04/16/2018 11:52 AM, Erik Skultety wrote:
> On Fri, Apr 13, 2018 at 04:47:14PM +0200, Michal Privoznik wrote:
>> So far we are repeating the following lines over and over:
>>
>>   virClassNew(virClassForObject(),
>>   "virSomeObject",
>>   sizeof(virSomeObject),
>>   virSomeObjectDispose);
>>
>> While this works, it is impossible to do some checking. Firstly,
>> the class name (the 2nd argument) doesn't match the name in the
>> code in all cases (the 3rd argument). Secondly, the current style
>> is needlessly verbose. This commit turns example into following:
>>
>>   VIR_CLASS_NEW(virClassForObject(),
>> virSomeObject);
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/access/viraccessmanager.c   |   6 +-
>>  src/bhyve/bhyve_conf.c  |   6 +-
>>  src/conf/capabilities.c |   6 +-
>>  src/conf/domain_capabilities.c  |  12 +--
>>  src/conf/domain_conf.c  |  18 ++---
>>  src/conf/domain_event.c | 126 
>> +++-
>>  src/conf/network_event.c|  12 +--
>>  src/conf/node_device_event.c|  18 ++---
>>  src/conf/object_event.c |  12 +--
>>  src/conf/secret_event.c |  18 ++---
>>  src/conf/storage_event.c|  18 ++---
>>  src/conf/virdomainobjlist.c |   6 +-
>>  src/conf/virinterfaceobj.c  |  12 +--
>>  src/conf/virnetworkobj.c|  12 +--
>>  src/conf/virnodedeviceobj.c |  12 +--
>>  src/conf/virsecretobj.c |  12 +--
>>  src/conf/virstorageobj.c|  24 ++
>>  src/datatypes.c |   6 +-
>>  src/interface/interface_backend_netcf.c |   6 +-
>>  src/libvirt-admin.c |   6 +-
>>  src/libxl/libxl_conf.c  |   6 +-
>>  src/libxl/libxl_domain.c|   6 +-
>>  src/libxl/libxl_migration.c |   6 +-
>>  src/logging/log_handler.c   |   6 +-
>>  src/lxc/lxc_conf.c  |   6 +-
>>  src/lxc/lxc_monitor.c   |   6 +-
>>  src/node_device/node_device_udev.c  |   6 +-
>>  src/qemu/qemu_agent.c   |   6 +-
>>  src/qemu/qemu_capabilities.c|   6 +-
>>  src/qemu/qemu_conf.c|   6 +-
>>  src/qemu/qemu_domain.c  |  36 +++--
>>  src/qemu/qemu_monitor.c |   6 +-
>>  src/rpc/virkeepalive.c  |   6 +-
>>  src/rpc/virnetclient.c  |   6 +-
>>  src/rpc/virnetclientprogram.c   |   6 +-
>>  src/rpc/virnetclientstream.c|   6 +-
>>  src/rpc/virnetdaemon.c  |   6 +-
>>  src/rpc/virnetlibsshsession.c   |   6 +-
>>  src/rpc/virnetsaslcontext.c |  12 +--
>>  src/rpc/virnetserver.c  |   6 +-
>>  src/rpc/virnetserverclient.c|   6 +-
>>  src/rpc/virnetserverprogram.c   |   6 +-
>>  src/rpc/virnetserverservice.c   |   6 +-
>>  src/rpc/virnetsocket.c  |   6 +-
>>  src/rpc/virnetsshsession.c  |   6 +-
>>  src/rpc/virnettlscontext.c  |  12 +--
>>  src/security/security_manager.c |   6 +-
>>  src/util/virclosecallbacks.c|   6 +-
>>  src/util/virdnsmasq.c   |   7 +-
>>  src/util/virfdstream.c  |   6 +-
>>  src/util/virfilecache.c |   6 +-
>>  src/util/virhash.c  |   6 +-
>>  src/util/virhostdev.c   |   6 +-
>>  src/util/viridentity.c  |   6 +-
>>  src/util/virmacmap.c|   6 +-
>>  src/util/virmdev.c  |   6 +-
>>  src/util/virobject.c|  12 +--
>>  src/util/virobject.h|   4 +
>>  src/util/virpci.c   |   6 +-
>>  src/util/virportallocator.c |   6 +-
>>  src/util/virresctrl.c   |  12 +--
>>  src/util/virscsi.c  |   6 +-
>>  src/util/virscsivhost.c |   6 +-
>>  src/util/virusb.c   |   6 +-
>>  src/vbox/vbox_common.c  |   6 +-
>>  src/vz/vz_driver.c  |   6 +-
>>  tests/virfilecachetest.c|   6 +-
> 
> ...
> 
>> diff --git a/src/datatypes.c b/src/datatypes.c
>> index 0c3c66a9ce..3016e45076 100644
>> --- a/src/datatypes.c
>> +++ b/src/datatypes.c
>> @@ -74,10 +74,8 @@ static int
>>  virDataTypesOnceInit(void)
>>  {
>>  #define DECLARE_CLASS_COMMON(basename, parent) \
>> -if (!(basename ## Class = virClassNew(parent, \
>> -  #basename, \
>> -  sizeof(basename), \
>> -  basename ## Dispose))) \
>> +if (!(basename ## Class = VIR_CLASS_NEW(parent, \
>> +   

Re: [libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[Michal Privoznik]

On 04/16/2018 01:53 PM, John Ferlan wrote:
> 
> 
> On 04/13/2018 10:47 AM, Michal Privoznik wrote:
>> In next patches this name will be needed for a different memeber.
> 
> s/memeber/member
> 
>> Also, it makes sense to rename the variable because it does not
>> contain reference to parent device, just its name.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/conf/virnodedeviceobj.c  | 2 +-
>>  src/datatypes.c  | 2 +-
>>  src/datatypes.h  | 2 +-
>>  src/libvirt-nodedev.c| 6 +++---
>>  src/node_device/node_device_driver.c | 4 ++--
>>  src/test/test_driver.c   | 6 +++---
>>  6 files changed, 11 insertions(+), 11 deletions(-)
>>
> 
> [...]
> 
> FWIW: Not sure about changing the accessor's name too - that would mean
> the API name doesn't match (virNodeDeviceGetParent which could lead to
> other problems with automatic code generation).

Not sure what do you mean by code generation. But that reminds me I
forgot to rename variable on RPC protocol level :-)

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/9] util: Make structs follow our naming convention

[Michal Privoznik]

On 04/16/2018 09:55 AM, Erik Skultety wrote:
> On Fri, Apr 13, 2018 at 04:47:09PM +0200, Michal Privoznik wrote:
>> There are two structs virMacMap and virFDStreamData that don't
>> have the underscore prefix. Put it there so that they follow the
>> rest of the code.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/virfdstream.c | 4 ++--
>>  src/util/virmacmap.c   | 2 +-
>>  src/util/virmacmap.h   | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> these suffer from the same problem:
> daemonAdmClientPrivate
> virDomainQemuMonitorEventData
> xentoollog_logger_libvirt (no convention here at all, so I'd skip this one)
> virLXCMeminfo
> qemuBlockNodeNameBackingChainData
> daemonClientStream
> virNetMessageHeader
> virNetMessageError
> virCgroup
> virNetlinkCallbackData
> virPerf
> virPerfEvent
> virPerfEventAttr
> virRotatingFileWriterEntry
> virRotatingFileReaderEntry
> virRotatingFileWriter
> virRotatingFileReader
> virMutex
> virRWLock
> virCond
> virThreadLocal
> virThread
> virTypedParameterRemoteValue (the second typedef is completely wrong):
> typedef struct _virTypedParameterRemoteValue virTypedParameterRemoteValue;
> typedef struct virTypedParameterRemoteValue 
> *virTypedParameterRemoteValuePtr;
> virOnceControl
> vbox - basically all of the structs :P
> vzDomObj
> 
> Honestly, given the number of places where this should be fixed, I'm not sure
> whether we should really go with the patch, but at the same time, I can 
> imagine
> us having this unified once and for all.

Actually, I don't need this patch. So I'm dropping it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[Michal Privoznik]

On 04/16/2018 09:26 AM, Erik Skultety wrote:
> On Fri, Apr 13, 2018 at 04:47:10PM +0200, Michal Privoznik wrote:
>> In next patches this name will be needed for a different memeber.
>> Also, it makes sense to rename the variable because it does not
>> contain reference to parent device, just its name.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/conf/virnodedeviceobj.c  | 2 +-
>>  src/datatypes.c  | 2 +-
>>  src/datatypes.h  | 2 +-
>>  src/libvirt-nodedev.c| 6 +++---
>>  src/node_device/node_device_driver.c | 4 ++--
>>  src/test/test_driver.c   | 6 +++---
>>  6 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
>> index ad0f27ee47..9d2996046f 100644
>> --- a/src/conf/virnodedeviceobj.c
>> +++ b/src/conf/virnodedeviceobj.c
>> @@ -870,7 +870,7 @@ virNodeDeviceObjListExportCallback(void *payload,
>>  virNodeDeviceMatch(obj, data->flags)) {
>>  if (data->devices) {
>>  if (!(device = virGetNodeDevice(data->conn, def->name)) ||
>> -VIR_STRDUP(device->parent, def->parent) < 0) {
>> +VIR_STRDUP(device->parentName, def->parent) < 0) {
>>  virObjectUnref(device);
>>  data->error = true;
>>  goto cleanup;
>> diff --git a/src/datatypes.c b/src/datatypes.c
>> index f7eef24ba8..0c3c66a9ce 100644
>> --- a/src/datatypes.c
>> +++ b/src/datatypes.c
>> @@ -653,7 +653,7 @@ virNodeDeviceDispose(void *obj)
>>  VIR_DEBUG("release dev %p %s", dev, dev->name);
>>
>>  VIR_FREE(dev->name);
>> -VIR_FREE(dev->parent);
>> +VIR_FREE(dev->parentName);
>>
>>  virObjectUnref(dev->conn);
>>  }
>> diff --git a/src/datatypes.h b/src/datatypes.h
>> index 1a8ea01ba3..66733b075c 100644
>> --- a/src/datatypes.h
>> +++ b/src/datatypes.h
>> @@ -618,7 +618,7 @@ struct _virNodeDevice {
>>  virObject object;
>>  virConnectPtr conn; /* pointer back to the connection */
>>  char *name; /* device name (unique on node) */
>> -char *parent;   /* parent device name */
>> +char *parentName;   /* parent device name */
>>  };
>>
>>  /**
>> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
>> index 563ce889b9..8ced3cea0e 100644
>> --- a/src/libvirt-nodedev.c
>> +++ b/src/libvirt-nodedev.c
>> @@ -346,16 +346,16 @@ virNodeDeviceGetParent(virNodeDevicePtr dev)
>>
>>  virCheckNodeDeviceReturn(dev, NULL);
>>
>> -if (!dev->parent) {
>> +if (!dev->parentName) {
>>  if (dev->conn->nodeDeviceDriver && 
>> dev->conn->nodeDeviceDriver->nodeDeviceGetParent) {
>> -dev->parent = 
>> dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);
>> +dev->parentName = 
>> dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);
> 
> Since you're adjusting the struct member name, you could go as far as fixing
> the *GetParent accessor's name too.

I can't. That is a public API and as such it cannot change.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 8/9] virobject: Check if @parent is the first member in class

[Michal Privoznik]

On 04/16/2018 02:30 PM, Erik Skultety wrote:
> On Fri, Apr 13, 2018 at 04:47:15PM +0200, Michal Privoznik wrote:
>> Our virObject code relies heavily on the fact that the first
>> member of the class struct is type of virObject (or some
>> derivation of if). Let's check for that.
> 
> If a class is missing 'parent' memeber, it's a bug in the definition of the
> struct/class, therefore there should be a static assertion rather than a
> runtime check.

If a class is missing parent then you'd hit compile time error because
offsetof() is trying to get offset of a non-existent member.

> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/virobject.c | 31 +--
>>  src/util/virobject.h |  5 -
>>  2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/util/virobject.c b/src/util/virobject.c
>> index c5a98d21cc..e184f5349e 100644
>> --- a/src/util/virobject.c
>> +++ b/src/util/virobject.c
>> @@ -77,6 +77,7 @@ virObjectOnceInit(void)
>>  {
>>  if (!(virObjectClass = virClassNew(NULL,
>> "virObject",
>> +   0,
>> sizeof(virObject),
>> NULL)))
> 
> Also, I don't like this extra parameter, which really shouldn't be needed; you
> created a macro which hides this parameter, but that doesn't mean that
> design-wise it makes sense to have it there, think of it as a constructor, you
> don't pass a constructor an offset of the class' member, because it shouldn't
> have need for it, but you do, solely for the purpose of checking whether we 
> have
> a particular member in place.
> So, to start a discussion about this (I also think Dan posted something 
> related
> to this recently, but I don't seem to be able to find it in the archives - do 
> I
> even archive?!!!), I came up with my first compile-time hack ever, it seems to
> work like expected, but I'd like to hear your opinions both the macro itself
> and the approach we're going to take, so here's my replacement patch:
> 
> diff --git a/src/util/virobject.h b/src/util/virobject.h
> index 92dd51239..2a973d401 100644
> --- a/src/util/virobject.h
> +++ b/src/util/virobject.h
> @@ -75,8 +75,12 @@ virClassPtr virClassForObjectRWLockable(void);
>  #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
>  # endif
> 
> +# define VIR_CLASS_HAS_PARENT(name) \
> +!sizeof(char[0-offsetof(name, parent)])

I don't quite understand why this is so obfuscated. Anyway, since
VIR_CLASS_NEW() is going to be a stand alone macro (like VIR_ENUM_DECL
for instance) we can do plain:

#define VIR_CLASS_NEW(prt, name) \
  verify(offsetof(name, parent) == 0); \
  if (!(name##Class = virClassNew(prt, #name, sizeof(name), name##Dispose))) \
return -1;

(written from the top of my head, not tested, not compiled, don't take
it too much literally)

We couldn't do that if VIR_CLASS_NEW() is still a function-like macro
( if (!(nameClass = VIR_CLASS_NEW(...))) return -1; ).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virsh: Clear vsh last error during virshCommandOptVolBy

[John Ferlan]

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

If one of the virStorageVolLookupBy{Key|Name|Path} succeeds and
we have a @vol, then clear the last libvirt error; otherwise, a
subsequent "other" failure may cause vshReportError to erroneously
report the wrong error as well as a reported 'vshError' error that
caused the failure.

Signed-off-by: John Ferlan 
---
 tools/virsh-volume.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index bacbec0d27..9d6ebd2325 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -129,6 +129,8 @@ virshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd,
 else
 vshError(ctl, _("failed to get vol '%s', specifying --%s "
 "might help"), n, pooloptname);
+} else {
+vshResetLibvirtError();
 }
 
 /* If the pool was specified, then make sure that the returned
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu_domain: Don't leak @paths in qemuDomainNamespaceSetupDisk

[Michal Privoznik]

Introduced in d3db304d2e4. Instead of returning immediately we
need to jump onto cleanup label where @paths is freed.

Signed-off-by: Michal Privoznik 
---

Pushed under trivial rule.

 src/qemu/qemu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7000de6a91..6e18886d07 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11199,7 +11199,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
 }
 
 if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0)
-return -1;
+goto cleanup;
 
 ret = 0;
  cleanup:
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 29/44] Deprecate QEMU_CAPS_DRIVE_CACHE_UNSAFE

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 07f7630655..9534eb6fae 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -954,8 +954,7 @@ mymain(void)
>  DO_TEST("disk-drive-cache-v2-wb", NONE);
>  DO_TEST("disk-drive-cache-v2-none", NONE);
>  DO_TEST("disk-drive-cache-directsync", NONE);
> -DO_TEST("disk-drive-cache-unsafe",
> -QEMU_CAPS_DRIVE_CACHE_UNSAFE);
> +DO_TEST("disk-drive-cache-unsafe", NONE);
>  DO_TEST("disk-drive-copy-on-read",
>  QEMU_CAPS_DRIVE_COPY_ON_READ);
>  DO_TEST("disk-drive-network-nbd", NONE);

We don't seem to test cache=unsafe anywhere else, so this will
have to stay.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 28/44] Deprecate QEMU_CAPS_NO_SHUTDOWN

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c6cfd05216..d175b196ea 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7486,17 +7486,10 @@ qemuProcessReconnect(void *opaque)
>  /* We can't get the monitor back, so must kill the VM
>   * to remove danger of it ending up running twice if
>   * user tries to start it again later
> - */
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) {
> -/* If we couldn't get the monitor and qemu supports
> - * no-shutdown, we can safely say that the domain
> - * crashed ... */
> -state = VIR_DOMAIN_SHUTOFF_CRASHED;
> -} else {
> -/* ... but if it doesn't we can't say what the state
> - * really is and FAILED means "failed to start" */
> -state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
> -}
> + * If we couldn't get the monitor and qemu supports
> + * no-shutdown, we can safely say that the domain
> + * crashed ... */

s/ and qemu/, since QEMU/

[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d451626be5..07f7630655 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1681,7 +1681,7 @@ mymain(void)
>  QEMU_CAPS_SCSI_LSI);
>  
>  DO_TEST("monitor-json", NONE);
> -DO_TEST("no-shutdown", QEMU_CAPS_NO_SHUTDOWN);
> +DO_TEST("no-shutdown", NONE);

I wonder if you could just drop the test case altogether, since
basically the whole test suite is using the feature now...

The same is actually true for the monitor-json test right above it
and probably a bunch more tests that were strictly connected to
capabilities you dropped or are going to drop. I should have
thought about that earlier, but it didn't really occur to me until
now and the series has already been partially pushed; I guess it's
a good idea to be more aggressive with the cleanup from this point
on, though. We can come back later and see whether we missed any
up until this point.

With the comment updated and the test case dropped,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 65/73] qemumigparamstest: Add basic test data

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:55PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
tests/qemumigparamsdata/basic.json  |  9 +
tests/qemumigparamsdata/basic.reply | 12 
tests/qemumigparamsdata/basic.xml   | 11 +++
tests/qemumigparamsdata/empty.json  |  3 +++
tests/qemumigparamsdata/empty.reply |  5 +
tests/qemumigparamsdata/empty.xml   |  4 
tests/qemumigparamstest.c   |  2 ++
7 files changed, 46 insertions(+)
create mode 100644 tests/qemumigparamsdata/basic.json
create mode 100644 tests/qemumigparamsdata/basic.reply
create mode 100644 tests/qemumigparamsdata/basic.xml
create mode 100644 tests/qemumigparamsdata/empty.json
create mode 100644 tests/qemumigparamsdata/empty.reply
create mode 100644 tests/qemumigparamsdata/empty.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 66/73] qemumigparamstest: Add test data for TLS parameters

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:56PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
tests/qemumigparamsdata/tls-enabled.json   | 11 +++
tests/qemumigparamsdata/tls-enabled.reply  | 14 ++
tests/qemumigparamsdata/tls-enabled.xml| 13 +
tests/qemumigparamsdata/tls-hostname.json  | 11 +++
tests/qemumigparamsdata/tls-hostname.reply | 14 ++
tests/qemumigparamsdata/tls-hostname.xml   | 13 +
tests/qemumigparamsdata/tls.json   | 11 +++
tests/qemumigparamsdata/tls.reply  | 14 ++
tests/qemumigparamsdata/tls.xml| 13 +
tests/qemumigparamstest.c  |  3 +++
10 files changed, 117 insertions(+)
create mode 100644 tests/qemumigparamsdata/tls-enabled.json
create mode 100644 tests/qemumigparamsdata/tls-enabled.reply
create mode 100644 tests/qemumigparamsdata/tls-enabled.xml
create mode 100644 tests/qemumigparamsdata/tls-hostname.json
create mode 100644 tests/qemumigparamsdata/tls-hostname.reply
create mode 100644 tests/qemumigparamsdata/tls-hostname.xml
create mode 100644 tests/qemumigparamsdata/tls.json
create mode 100644 tests/qemumigparamsdata/tls.reply
create mode 100644 tests/qemumigparamsdata/tls.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 64/73] tests: Add tests for QEMU migration parameters

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:54PM +0200, Jiri Denemark wrote:

This is an enhanced replacement for the original test from
qemumonitorjsontest which was dropped earlier in this series. More data
files with some real data will be added in the following patches.

Signed-off-by: Jiri Denemark 
---
tests/Makefile.am |  12 ++
tests/qemumigparamsdata/unsupported.json  |   3 +
tests/qemumigparamsdata/unsupported.reply |   7 +
tests/qemumigparamsdata/unsupported.xml   |   4 +
tests/qemumigparamstest.c | 237 ++
5 files changed, 263 insertions(+)
create mode 100644 tests/qemumigparamsdata/unsupported.json
create mode 100644 tests/qemumigparamsdata/unsupported.reply
create mode 100644 tests/qemumigparamsdata/unsupported.xml
create mode 100644 tests/qemumigparamstest.c



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 13/14] qemu_hotplug: Hotunplug of reservations

[Michal Privoznik]

On 04/14/2018 05:14 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> If we are the last one to use pr-manager object we need to remove
>> it and also kill the qemu-pr-helper process.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_hotplug.c | 38 ++
>>  1 file changed, 38 insertions(+)
>>
> 
> Again, my opinion this is combined too - patch is larger, but everything
> is covered at one time.
> 
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 43bb910ed6..98e1bf7082 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3894,6 +3894,34 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr 
>> def,
>>  }
>>  
>>  
>> +static qemuDomainDiskPRDPtr
>> +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
>> +   virDomainDiskDefPtr disk)
>> +{
>> +qemuDomainStorageSourcePrivatePtr srcPriv;
>> +size_t i;
>> +
>> +if (!virStoragePRDefIsManaged(disk->src->pr))
>> +return NULL;
>> +
>> +for (i = 0; i < vm->def->ndisks; i++) {
>> +const virDomainDiskDef *domainDisk = vm->def->disks[i];
>> +
>> +if (domainDisk == disk)
>> +continue;
>> +
>> +if (virStoragePRDefIsManaged(domainDisk->src->pr))
>> +break;
>> +}
>> +
>> +if (i != vm->def->ndisks)
>> +return NULL;
>> +
>> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +return srcPriv->prd;
>> +}
>> +
>> +
> 
> Trying to combine two separate things into one?
> 
> Thing 1 -> Remove the object from the domain
> Thing 2 -> Kill the PRD helper *iff* this is the last one using it
> 
>>  static int
>>  qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> @@ -3907,6 +3935,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>>  char *drivestr;
>>  char *objAlias = NULL;
>>  char *encAlias = NULL;
>> +qemuDomainDiskPRDPtr prd = NULL;
>>  
>>  VIR_DEBUG("Removing disk %s from domain %p %s",
>>disk->info.alias, vm, vm->def->name);
>> @@ -3944,6 +3973,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>>  }
>>  }
>>  
>> +prd = qemuDomainDiskNeedRemovePR(vm, disk);
>> +
>>  qemuDomainObjEnterMonitor(driver, vm);
>>  
>>  qemuMonitorDriveDel(priv->mon, drivestr);
>> @@ -3959,6 +3990,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>>  ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
>>  VIR_FREE(encAlias);
>>  
>> +/* If it fails, then so be it - it was a best shot */
>> +if (prd)
>> +ignore_value(qemuMonitorDelObject(priv->mon, prd->alias));
>> +
> 
> OK - remove the object if we have one...
> 
>>  if (disk->src->haveTLS)
>>  ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
>>  
>> @@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>>  }
>>  }
>>  
>> +if (prd)
>> +qemuProcessKillPRDaemon(vm);
>> +
> 
> But we only want to kill if this if there are no other disk's needing
> the pr-helper, right?  So we need to have a routine that would be run
> after the disk is really removed.  The two phase removal process always
> gets me ;-)  - that is Detach vs. Remove and which one is last *after*
> the disk is removed from the list of disks *and* when we know the object
> has been removed from the domain, then we can stop the PR process iff
> this is the last one using it.

Detach is called first and then, when guest OS stopped using the device
(disk in this case) and so did qemu Remove is called. And we have to do
the steps I described in 12/14 but in reverse order:

1) remove the disk (done already, no special handling from this feature
POV),
2) remove pr-manager object (if no other disk still uses it),
3) kill the pr-helper process.

The check from step 2) is done in qemuDomainDiskNeedRemovePR() and on
success (=kill & remove) a non-NULL pointer is returned.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 07/14] qemu_cgroup: Allow /dev/mapper/control for PR

[Michal Privoznik]

On 04/14/2018 02:55 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> Just like in previous commit, qemu-pr-helper might want to open
>> /dev/mapper/control under certain circumstances. Therefore we
>> have to allow it in cgroups.
> 
> Perhaps the two patches should be combined then...  yes, I know cgroups
> is different than namespace, so I understand the separation.

Yeah, I'd like to keep them separated. Even though they allow access to
the same path they do that in different areas of the code.

> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_cgroup.c  | 33 ++---
>>  src/util/virdevmapper.c |  8 +++-
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index d88eb7881f..546a4c8e63 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>>  }
>>  
>>  
>> +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
>> +
> 
> Almost too bad we didn't have a common place for this in some existing
> included .h file.

Yeah :(

> 
>>  static int
>>  qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>>   virStorageSourcePtr src,
>> @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>>  return 0;
>>  }
>>  
>> +if (virStoragePRDefIsManaged(src->pr) &&
>> +qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
>> +return -1;
>> +
> 
> Could the above be done potentially many times?  Could be expensive, no?
>  Considering what virDevMapperGetTargets[Impl] does...

It shouldn't be expensive. At the very first call of
virDevMapperGetTargetsImpl() we get ENXIO (this the change below) and
thus there only very small time penalty added.

> 
>>  return qemuSetupImagePathCgroup(vm, src->path, src->readonly || 
>> forceReadonly);
>>  }
>>  
>> @@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>>  virStorageSourcePtr src)
>>  {
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>> -int perms = VIR_CGROUP_DEVICE_READ |
>> -VIR_CGROUP_DEVICE_WRITE |
>> -VIR_CGROUP_DEVICE_MKNOD;
>> +int perms = VIR_CGROUP_DEVICE_RWM;
>> +size_t i;
>>  int ret;
>>  
>>  if (!virCgroupHasController(priv->cgroup,
>> @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>>  return 0;
>>  }
>>  
>> +for (i = 0; i < vm->def->ndisks; i++) {
>> +virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
>> +
>> +if (src == diskSrc)
>> +continue;
>> +
>> +if (virStoragePRDefIsManaged(diskSrc->pr))
>> +break;
>> +}
>> +
>> +if (i == vm->def->ndisks) {
> 
> If there was only one that's managed and it's @src, then we don't get
> here...
> 

How come? Say there are 3 disks for a domain and the first one is
managed: disks = {A, B, C} (where A.managed = yes}.

So the loop goes like this:

  qemuTeardownImageCgroup(A):
i = 0;
if (A.src == disks[0].src) //true
  continue;

i = 1;
if (A.src == disks[1].src) //false
  continue;

if (isManaged(disks[1]) //false
  break;

i = 2;
if (A.src == disks[2].src) //false
  continue;

if (isManaged(disks[2]) //false
  break;

// Here, i == ndisks == 3;

Or am I missing something?


>> +VIR_DEBUG("Disabling device mapper control");
>> +ret = virCgroupDenyDevicePath(priv->cgroup,
>> +  DEVICE_MAPPER_CONTROL_PATH, perms, 
>> true);
> 
> You go through great lengths to ensure this is only done once converse
> to the qemuSetupImageCgroupInternal change...

Yes, because we can't deny helper the access if there's still another
disk that might be using it (note that only managed disks use helper
which runs in qemu's namespace/cgroup context). But we can allow it
multiple times.

> 
> I guess I would think that if we can determine at Prepare time that NS
> and cgroups would need to add /dev/mapper/control and we only want to do
> that config once, then we should be able
> 
> Yes, of course we then we potentially miss adding for hotplug. Honestly
> it seems more of a "global" to qemu_domain rather than "local" to a
> particular disk source even though it's needed by the local disk source,
> is it really something the disk source itself (or it's private data
> structure) should be managing?

Well, I view qemuSetupImageCgroup() and qemuTeardownImageCgroup() as
entry points to CGroup mgmt. Therefore it's less lines of code to do
decision there. Doing it anywhere else would be impractical IMO.

> 
>> +virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
>> + DEVICE_MAPPER_CONTROL_PATH,
>> + virCgroupGetDevicePermsString(perms), ret);
>> +

Re: [libvirt] [PATCH v4 09/14] qemu: Introduce pr_helper to qemu.conf

[Michal Privoznik]

On 04/14/2018 03:35 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> Just like we allow users overriding path to bridge-helper
>> detected at compile time we can allow them to override path to
>> qemu-pr-helper.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  m4/virt-driver-qemu.m4 | 5 +
>>  src/qemu/libvirtd_qemu.aug | 1 +
>>  src/qemu/qemu.conf | 4 
>>  src/qemu/qemu_conf.c   | 7 ++-
>>  src/qemu/qemu_conf.h   | 1 +
>>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>>  6 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
>> index b9bafdab90..80e1d3ad46 100644
>> --- a/m4/virt-driver-qemu.m4
>> +++ b/m4/virt-driver-qemu.m4
>> @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
>> [/usr/libexec:/usr/lib/qemu:/usr/lib])
>>AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"],
>>   [QEMU bridge helper])
>> +  AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper],
>> +   [/usr/bin/qemu-pr-helper],
>> +   [/usr/bin:/usr/libexec])
> 
> So the default install location of qemu-pr-helper is /usr/bin unlike
> bridge-helper which is /usr/libexec?

Yes. I guess this has something to do with aforementioned systemd socket
activation.

> 
>> +  AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"],
>> + [QEMU PR helper])
>>  ])
>>  
>>  AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
>> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
>> index c19bf3a43a..2dc16e91fd 100644
>> --- a/src/qemu/libvirtd_qemu.aug
>> +++ b/src/qemu/libvirtd_qemu.aug
>> @@ -86,6 +86,7 @@ module Libvirtd_qemu =
>> let process_entry = str_entry "hugetlbfs_mount"
>>   | bool_entry "clear_emulator_capabilities"
>>   | str_entry "bridge_helper"
>> + | str_entry "pr_helper"
>>   | bool_entry "set_process_name"
>>   | int_entry "max_processes"
>>   | int_entry "max_files"
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index 07eab7efff..30fdd54e2c 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -775,3 +775,7 @@
>>  # This directory is used for memoryBacking source if configured as file.
>>  # NOTE: big files will be stored here
>>  #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
>> +
>> +# Path to the SCSI persistent reservations helper. This helper is
>> +# used whenever  are enabled for SCSI disks.
> 
> s/disks/LUN devices/
> 
>> +#pr_helper = "/usr/libexec/qemu-pr-helper"
> 
> Going with my note above - the default path in the m4 file shows
> /usr/bin first... So should this match that? Similar to how
> qemu-bridge-helper matches the /usr/libexec install location?

Yeah, this is a leftover from previous rounds where I had /usr/libexec/.
I'm changing this to /usr/bin/.

> 
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 36cf3a281c..8c69dbe75c 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
>> privileged)
>>  goto error;
>>  }
>>  
>> -if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0)
>> +if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 ||
>> +VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
>>  goto error;
>>  
>>  cfg->clearEmulatorCapabilities = true;
>> @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>>  }
>>  VIR_FREE(cfg->hugetlbfs);
>>  VIR_FREE(cfg->bridgeHelperName);
>> +VIR_FREE(cfg->prHelperName);
>>  
>>  VIR_FREE(cfg->saveImageFormat);
>>  VIR_FREE(cfg->dumpImageFormat);
>> @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
>> cfg,
>>  if (virConfGetValueString(conf, "bridge_helper", 
>> >bridgeHelperName) < 0)
>>  goto cleanup;
>>  
>> +if (virConfGetValueString(conf, "pr_helper", >prHelperName) < 0)
>> +goto cleanup;
>> +
>>  if (virConfGetValueBool(conf, "mac_filter", >macFilter) < 0)
>>  goto cleanup;
>>  
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index e1ad5463f3..7a63780c48 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig {
>>  size_t nhugetlbfs;
>>  
>>  char *bridgeHelperName;
>> +char *prHelperName;
>>  
>>  bool macFilter;
>>  
>> diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
>> b/src/qemu/test_libvirtd_qemu.aug.in
>> index 688e5b9fda..c0efae47bd 100644
>> --- a/src/qemu/test_libvirtd_qemu.aug.in
>> +++ b/src/qemu/test_libvirtd_qemu.aug.in
>> @@ -100,3 +100,4 @@ module Test_libvirtd_qemu =
>>  { "1" = "mount" }
>>  }
>>  { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
>> +{ "pr_helper" = 

Re: [libvirt] [PATCH v4 08/14] qemu: Generate cmd line at startup

[Michal Privoznik]

On 04/14/2018 03:04 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> This is the easier part. All we need to do here is put -object
>> pr-manager-helper,id=$alias,path=$socketPath and then just
>> reference the object in -drive file.pr-manager=$alias.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_command.c| 94 
>> ++
>>  src/qemu/qemu_command.h|  3 +
>>  .../disk-virtio-scsi-reservations.args | 35 
>>  tests/qemuxml2argvtest.c   |  4 +
>>  4 files changed, 136 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>>
> 
> This patch fails qemuxml2argv for me because of commits 'cc32731a' and
> 'a32539de'...
> 

Oh yeah. There's always chance that between me posting patches and
review somebody will push something that invalidates my patches.


>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 514c3ab2ef..81f6025207 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>>  }
>>  
>>  
>> +static void
>> +qemuBuildDriveSourcePR(virBufferPtr buf,
>> +   virStorageSourcePtr src)
>> +{
>> +qemuDomainStorageSourcePrivatePtr srcPriv = 
>> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>> +qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +if (!prd || !prd->alias)
>> +return;
>> +
>> +virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
>> +}
>> +
>> +
>>  static int
>>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>  virQEMUCapsPtr qemuCaps,
>> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>  
>>  if (disk->src->debug)
>>  virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
>> +
>> +qemuBuildDriveSourcePR(buf, disk->src);
>>  } else {
>>  if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>>  goto cleanup;
>> @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>>  }
>>  
>>  
>> +/**
>> + * qemuBuildPRManagerInfoProps:
>> + * @prd: disk PR runtime info
>> + * @propsret: JSON properties to return
>> + *
>> + * Build the JSON properties for the pr-manager object.
>> + *
>> + * Returns: 0 on success (@propsret is NULL if no properties are needed),
>> + * -1 on failure (with error message set).
>> + */
>> +int
>> +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
>> +virJSONValuePtr *propsret)
>> +{
>> +*propsret = NULL;
>> +
>> +if (!prd || !prd->alias)
>> +return 0;
>> +
>> +if (virJSONValueObjectCreate(propsret,
>> + "s:path", prd->path,
>> + NULL) < 0)
>> +return -1;
>> +
>> +return 0;
>> +}
>> +
>> +
>> +static int
>> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>> + const virDomainDef *def)
>> +{
>> +size_t i;
>> +bool managedAdded = false;
>> +virJSONValuePtr props = NULL;
>> +char *tmp = NULL;
>> +int ret = -1;
>> +
>> +for (i = 0; i < def->ndisks; i++) {
>> +const virDomainDiskDef *disk = def->disks[i];
>> +qemuDomainStorageSourcePrivatePtr srcPriv = 
>> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +
>> +if (virStoragePRDefIsManaged(disk->src->pr)) {
>> +if (managedAdded)
>> +continue;
>> +
>> +managedAdded = true;
> 
> As soon as we find one that needs managing we should break from the loop
> and then only add pr-manager-helper if we have one to manage. No sense
> going through the rest of the list of disks.  Move @disk into the top
> level and then use it to decide whether or not to add the object.  Then
> in some way signify that the object was added so that future hotplugs
> wouldn't need to somehow guess - a simple check that it was added
> already would seem to suffice.

Not true. We need to add manager objects even for cases where libvirt is
not managing the helper process. For instance, for the following XML:

  

  

  

  

the following cmd line needs to be generated:

  -object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
  path=/path/to/qemu-pr-helper.sock \

otherwise qemu would now know where to connect. However, if libvirt is
managing the pr-helper process, all the managed disks share the same
process => object on the command line => it must be added exactly once.

> 
>> +}
> 
> The next hunk would seem to be useful for the hotplug path, no?
> Including perhaps setting some sort of qemu_domain level boolean that
> the object was added already so that subsequent or consecutive hotplugs
> wouldn't try to add 

Re: [libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup

[Michal Privoznik]

On 04/14/2018 04:56 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> Before we exec() qemu we have to spawn pr-helper processes for
>> all managed reservations (well, technically there can only one).
> 
> Don't mince words - what have to spawn the qemu-pr-helper process for
> all managed persistent reservations.
> 
>> The only caveat there is that we should place the process into
>> the same namespace and cgroup as qemu (so that it shares the same
>> view of the system). But we can do that only after we've forked.
>> That means calling the setup function between fork() and exec().
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_process.c | 224 
>> 
>>  1 file changed, 224 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index f02114c693..982c989a0a 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -2556,6 +2556,223 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
>>  ret = 0;
>>   cleanup:
>>  virObjectUnref(caps);
>> +
>> +return ret;
>> +}
>> +
>> +
>> +static char *
>> +qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm,
>> +const char *prdAlias)
>> +{
>> +qemuDomainObjPrivatePtr priv = vm->privateData;
>> +
>> +return virPidFileBuildPath(priv->libDir, prdAlias);
>> +}
>> +
>> +
>> +static void
>> +qemuProcessKillPRDaemon(virDomainObjPtr vm)
>> +{
>> +virErrorPtr orig_err;
>> +char *prAlias;
>> +char *prPidfile;
> 
> I would hope that we'd be able to figure out in some way that we
> actually started this for the the domain.
> 
>> +
>> +if (!(prAlias = qemuDomainGetManagedPRAlias())) {
>> +VIR_WARN("Unable to get default PR manager alias");
>> +return;
>> +}
>> +
>> +if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) {
> 
> Since we know that we're using the managed one, then we should be able
> to pass/formulate the "static" prAlias here rather than possibly failing
> because we don't have enough memory above and then need to free it
> immediately afterwards.

Yeah, well if there's not enough memory to allocate 11 bytes there's
probably not enough memory to do any stuff done below, i.e.
qemuProcessBuildPRHelperPidfilePath(), or any syscall done in
virPidFileForceCleanupPath().

> 
>> +VIR_WARN("Unable to construct pr-helper pidfile path");
>> +VIR_FREE(prAlias);
>> +return;
>> +}
>> +VIR_FREE(prAlias);
>> +
>> +virErrorPreserveLast(_err);
>> +if (virPidFileForceCleanupPath(prPidfile) < 0) {
>> +VIR_WARN("Unable to kill pr-helper process");
>> +} else if (unlink(prPidfile) < 0 &&
>> +   errno != ENOENT) {
>> +virReportSystemError(errno,
>> + _("Unable to remove stale pidfile %s"),
>> + prPidfile);
>> +}
>> +virErrorRestore(_err);
>> +
>> +VIR_FREE(prPidfile);
>> +}
>> +

>> +static int
>> +qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm)
>> +{
>> +size_t i;
>> +int ret = -1;
>> +qemuDomainDiskPRDPtr prd = NULL;
>> +
>> +for (i = 0; i < vm->def->ndisks; i++) {
>> +const virDomainDiskDef *disk = vm->def->disks[i];
>> +qemuDomainStorageSourcePrivatePtr srcPriv;
>> +
>> +if (!virStoragePRDefIsManaged(disk->src->pr))
>> +continue;
>> +
>> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +prd = srcPriv->prd;
>> +break;
>> +}
> 
> Now that we know we needed to start one, then once we're successful we
> should be able to store that we did start it... and that could be added
> to some sort of private structure.  This is where I could see a private
> data being used.

Well, we can have that when qemu finally implements events. Then we can
keep this internal state in sync with reality.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/14] qemu_hotplug: Hotplug of reservations

[Michal Privoznik]

On 04/14/2018 05:14 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> When attaching a disk that requires pr-manager we might need to
>> plug the pr-manager object too.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_hotplug.c | 41 +
>>  1 file changed, 41 insertions(+)
>>
> 
> My opinion - combine w/ previous patch.
> 
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 468153c79c..43bb910ed6 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -391,6 +391,29 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
>>  }
>>  
>>  
>> +static int
>> +qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
>> + qemuDomainDiskPRDPtr prd,
>> + virJSONValuePtr *propsret)
>> +{
>> +size_t i;
>> +
>> +*propsret = NULL;
>> +
>> +for (i = 0; i < vm->def->ndisks; i++) {
>> +const virDomainDiskDef *domainDisk = vm->def->disks[i];
>> +
>> +if (virStoragePRDefIsManaged(domainDisk->src->pr)) {
>> +/* qemu-pr-helper should be already started because
>> + * another disk in domain requires it. */
>> +return 0;
>> +}
>> +}
> 
> I don't understand the need for the above hunk - that was done in
> patch11 wasn't it?  This is why I like things combined - going back and
> forth and changing logic as patches are developed means something may be
> missed.


Okay the comment might be misleading. It should be s/started/added/.

Anwyay, when adding a disk that has PR configured we need to:

1) start pr-helper process (if we are the ones managing it),
2) construct JSON for pr-manager object,
3) add pr-mananager object on the monitor,
4) finally, add disk on the monitor.

And we need to do it in this order, otherwise qemu might try to connect
to not yet running process. Now, obviously, since there can be only one
managed pr-helper process per domain, there can be only one pr-manager
object. Therefore, when somebody is trying to hotplug a disk with PR
enabled & managed, but there already is a disk like that: step 1) turns
into no-op (see previous patch), and we also need steps 2) and 3) to
turn into no-op because the object is already in qemu. Trying to add it
again would result in error.

> 
>> +
>> +return qemuBuildPRManagerInfoProps(prd, propsret);
>> +}
>> +
>> +
>>  /**
>>   * qemuDomainAttachDiskGeneric:
>>   *
>> @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  bool driveAdded = false;
>>  bool secobjAdded = false;
>>  bool encobjAdded = false;
>> +bool prmgrAdded = false;
>>  bool prdStarted = false;
>>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  virJSONValuePtr secobjProps = NULL;
>>  virJSONValuePtr encobjProps = NULL;
>> +virJSONValuePtr prmgrProps = NULL;
>>  qemuDomainStorageSourcePrivatePtr srcPriv;
>>  qemuDomainSecretInfoPtr secinfo = NULL;
>>  qemuDomainSecretInfoPtr encinfo = NULL;
>> +qemuDomainDiskPRDPtr prd = NULL;
>>  
>>  if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
>>  goto cleanup;
>> @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  if (srcPriv) {
>>  secinfo = srcPriv->secinfo;
>>  encinfo = srcPriv->encinfo;
>> +prd = srcPriv->prd;
> 
> As noted much earlier - the private data isn't needed here as the prd is
> in the disk storage definition.
> 
>>  }
>>  
>>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  if (encinfo && qemuBuildSecretInfoProps(encinfo, ) < 0)
>>  goto error;
>>  
>> +if (qemuMaybeBuildPRManagerInfoProps(vm, prd, ) < 0)
>> +goto error;
>> +
> 
> This should just call qemuBuildPRManagerInfoProps directly since it
> already checks if !prd || !prd->alias

No, because we might end up trying to add pr-manager object that is
already there in qemu which would fail and so would whole hotplug opertaion.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 06/14] qemu_ns: Allow /dev/mapper/control for PR

[Michal Privoznik]

On 04/14/2018 02:15 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> If qemu-pr-helper is compiled with multipath support the first
>> thing it does is opening /dev/mapper/control. Since we're going
> 
> s/opening/open
> 
>> to be running it inside qemu namespace we need to create it
>> there. Unfortunately, we don't know if it was compiled with or
>> without multipath so we have to create it anyway.
>>
> 
> Not sure this explains whether it's multipath or namespaces that's the
> focus/cause of this patch. I thought multipath allows multiple ways to
> address the same LUN and namespace provides a mechanism to "control"
> device events and protections so that something like udev doesn't change
> something behind our back. For PR it's a mechanism to allow certain
> ioctl calls to be able to control ownership and/or write processing to
> the LUN so that it's not possible to have two writers>
> So does this patch add /dev/mapper/control to the namespace list because
> that's what will "control" the ioctl's to the LUN used by PR ???

No. It's because if compiled with multipath support, qemu-pr-helper uses
/dev/mapper/control to query devmapper version (dm_init()):

https://git.qemu.org/?p=qemu.git;a=blob;f=scsi/qemu-pr-helper.c;h=d0f83176e1a63547a28282849c228cf2e7acd5a2;hb=HEAD#l1032

/dev/mapper/control is used to talk to devmapper (kernel) - in this
particular case to check if given device is a multipath target, not to
manipulate individual devices. Because if a device is multipath target
then lowlevel stuff looks different (see do_pr_in() and do_pr_out()).

Anyway, it shouldn't matter what pr-helper needs /dev/mapper/control
for. It does so we need to create it in the namespace and allow the
helper to access it.

> 
>> BTW: This might be the ugliest piece of code I've ever written
>> but @devMapperControl really needs to be type of char * otherwise
>> some crazy check in VIR_APPEND_ELEMENT fails.
> 
> This hunk probably doesn't belong in a commit message...  and well
> doesn't necessarily provide the confidence level for acceptance ;-)!

It's not because of allowing /dev/mapper/control but because of typecast
from const char * to char *.

> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_domain.c | 14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0856f04406..6fe4eb57e1 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -108,6 +108,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
>>  #define PROC_MOUNTS "/proc/mounts"
>>  #define DEVPREFIX "/dev/"
>>  #define DEV_VFIO "/dev/vfio/vfio"
>> +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
>>  
>>  
>>  struct _qemuDomainLogContext {
>> @@ -10269,6 +10270,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
>> ATTRIBUTE_UNUSED,
>>  goto cleanup;
>>  }
>>  
>> +/* qemu-pr-helper might require access to /dev/mapper/control. */
>> +if (virStoragePRDefIsEnabled(disk->src->pr) &&
>> +qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
>> +goto cleanup;
>> +
>>  ret = 0;
>>   cleanup:
>>  VIR_FREE(dst);
>> @@ -11281,6 +11287,9 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>  const char **paths = NULL;
>>  size_t npaths = 0;
>>  int ret = -1;
>> +/* This is very nasty but we need it to work around some
>> + * stupid checks in VIR_APPEND_ELEMENT macro. */
>> +char *devMapperControl = (char *) DEVICE_MAPPER_CONTROL_PATH;
> 
> alternatively
> 
> char *dmPath = NULL;
> 
>>  
>>  if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>>  return 0;
>> @@ -11296,6 +11305,11 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>  goto cleanup;
>>  }
>>  
>> +/* qemu-pr-helper might require access to /dev/mapper/control. */
>> +if (virStoragePRDefIsEnabled(src->pr) &&
>> +VIR_APPEND_ELEMENT_COPY(paths, npaths, devMapperControl) < 0)
> 
> Alternatively:
> 
> VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0) &&
> VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath)

Okay, I'll do this. It's cleaner.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/14] qemu_hotplug: Hotplug of reservations

[Michal Privoznik]

On 04/14/2018 05:18 PM, John Ferlan wrote:
> [...]
> 
>>   *
>> @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  bool driveAdded = false;
>>  bool secobjAdded = false;
>>  bool encobjAdded = false;
>> +bool prmgrAdded = false;
>>  bool prdStarted = false;
>>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  virJSONValuePtr secobjProps = NULL;
>>  virJSONValuePtr encobjProps = NULL;
>> +virJSONValuePtr prmgrProps = NULL;
>>  qemuDomainStorageSourcePrivatePtr srcPriv;
>>  qemuDomainSecretInfoPtr secinfo = NULL;
>>  qemuDomainSecretInfoPtr encinfo = NULL;
>> +qemuDomainDiskPRDPtr prd = NULL;
>>  
>>  if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
>>  goto cleanup;
>> @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  if (srcPriv) {
>>  secinfo = srcPriv->secinfo;
>>  encinfo = srcPriv->encinfo;
>> +prd = srcPriv->prd;
>>  }
>>  
>>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  if (encinfo && qemuBuildSecretInfoProps(encinfo, ) < 0)
>>  goto error;
>>  
>> +if (qemuMaybeBuildPRManagerInfoProps(vm, prd, ) < 0)
>> +goto error;
>> +
>>  if (disk->src->haveTLS &&
>>  qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src,
>>disk->info.alias) < 0)
>> @@ -484,6 +514,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>  encobjAdded = true;
>>  }
>>  
>> +if (prmgrProps) {
>> +rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", 
>> prd->alias,
>> +  prmgrProps);
>> +prmgrProps = NULL; /* qemuMonitorAddObject consumes */
>> +if (rv < 0)
>> +goto exit_monitor;
>> +prmgrAdded = true;
>> +}
>> +
> 
> Oh yeah - coverity let me know that @prmgrProps could be leaked if we
> don't get this far - need to add the virJSONValueFree cleanup

Ah, sure. Nice catch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 04/14] qemu: Generate alias and socket path for pr-helper

[Michal Privoznik]

On 04/13/2018 10:51 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> While we're not generating the command line just yet (look for
>> the next commits), we can generate the alias for pr-manager.
>> A domain can have up to one managed pr-manager (in which case
>> socket path is decided by libvirt and pr-helper is spawned by
>> libvirt too), but up to ndisks of unmanaged ones (one per disk
>> basically). In case of the former we can generate a simple alias
>> and be sure it'll not conflict. But in case of the latter to
> 
> Well if it's only one, then it had better not conflict (IOW: after the
> and is unnecessary because there is check).
> 
>> avoid any conflicts let's generate alias that's based on
>> something unique - like disk target.
>>
> 
> Make sure this commit message follows whatever happens in this patch...
> 
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/libvirt_private.syms  |  2 ++
>>  src/qemu/qemu_alias.c | 11 ++
>>  src/qemu/qemu_alias.h |  2 ++
>>  src/qemu/qemu_domain.c| 87 
>> +--
>>  src/qemu/qemu_domain.h| 10 ++
>>  src/util/virstoragefile.c | 15 
>>  src/util/virstoragefile.h |  2 ++
>>  7 files changed, 126 insertions(+), 3 deletions(-)
>>
> 
> This patch does two things - one is just the pure alias/path for the
> pr-helper for the active domain. The second is copy that same
> information to the private storage source, which I'm not sure (yet) why
> it's necessary since both can be regenerated as needed based on fairly
> static data as opposed to secinfo and encinfo which get filled in with
> information only available during Prepare (the interaction with the
> secret driver and the need for the @conn pointer) that wasn't available
> during launch/command line building. Although some of that has changed
> with more recent changes to be able to get a secret conn "on the fly".
> Anyway, I digress.
> 
> The point being - please note why you're also saving something in the
> private storage source area...  The 'path' is already present in the
> domain XML (both active and inactive) and the 'alias' could be saved in
> the active output if you tweaked virStoragePRDefFormat to match what
> virDomainDeviceInfoFormat does.

Okay. I will put it in the commit message. I thought we've discussed
this earlier. I rather put and info into status XML even though it might
look useless right now than having to write some crazy backcompat code
later.

> 
> 
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index a376e3bb0d..5b7b5514a2 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString;
>>  virStorageNetProtocolTypeToString;
>>  virStoragePRDefFormat;
>>  virStoragePRDefFree;
>> +virStoragePRDefIsEnabled;
>>  virStoragePRDefIsEqual;
>> +virStoragePRDefIsManaged;
>>  virStoragePRDefParseXML;
>>  virStorageSourceBackingStoreClear;
>>  virStorageSourceClear;
>> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
>> index 95d1e0370a..6db3da27a8 100644
>> --- a/src/qemu/qemu_alias.c
>> +++ b/src/qemu/qemu_alias.c
>> @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias)
>>  
>>  return ret;
>>  }
>> +
>> +
>> +char *
>> +qemuDomainGetManagedPRAlias(void)
>> +{
>> +char *alias;
>> +
>> +ignore_value(VIR_STRDUP(alias, "pr-helper0"));
>> +
>> +return alias;
>> +}
> 
> I don't really see a purpose for this function.  If it were to survive,
> then it could take a parameter "const char *srcalias" and create/return
> the alias from that based on what's in qemuDomainPrepareDiskPRD.

The purpose is to generate alias at only one place instead of several
virAsprintf-s scattered all around the code.

> 
> Eventually when the qemuProcessKillPRDaemon is created, it doesn't
> necessarily distinguish between managed and unmanaged...

Well, unmanaged helper should not have its socket in libvirt private
path. Under any circumstances. And what qemuProcessKillPRDaemon does is:
1) it constructs PID file path
2) and kills any pr-helper that might had been left behind.

I find this approach more robust than plain "check if there is a disk
with pr-helper and if so kill it" because if libvirt is ever split brained

> Still having it
> fail because it couldn't strdup what amounts to be a static string
> doesn't make much sense.

Well, there are couple of reasons for stdup()-ing a static string:
a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to
care if alias is a static string or dynamically allocated one,
b) higher memory consumption (by 11 bytes btw) is only for a short
period of time (when constructing PID file path). Although, the alias is
kept around for entire time domain is running.

And if libvirt is unable to allocate 11 bytes then you're in much bigger
trouble anyways.
> 
>  diff --git a/src/qemu/qemu_alias.h 

Re: [libvirt] [PATCH v4 05/14] qemu: Store pr runtime data in status XML

[Michal Privoznik]

On 04/13/2018 10:57 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> Now that we generate pr-manager alias and socket path store them
>> in status XML so that they are preserved across daemon restarts.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_domain.c | 64 
>> ++
>>  1 file changed, 64 insertions(+)
>>
> 
> So if we were to save this information (and at this point I don't think
> we need to), then this is where we should be allocating and filling in
> the private data (e.g. not in the previous patch).

How come? What would be left from the previous patch if private runtime
struct would be introduced only here? Or are you just suggesting
swapping these two patches?

> 
> Again as I noted previously, save the alias when printing the domain
> active information and I think you're good.

No, I don't want to expose info on PR helper more than is necessary. The
fact that it's a separate process should not be visible to users because
it is an implementation detail of QEMU. Other hypervisors might do this
differently. And even though it might not be visible from the patches,
using unmanaged mode is discouraged. In fact, unmanaged mode is on the
edge. If pr-helper is viewed as internal implementation the unmanaged
mode has no place in libvirt. However, qemu devels are experimenting
with systemd socket activation and for socket path must be configurable
through libvirt. So the only reason for using unmanaged PRs is systemd
socket activation.

Side note, we are not even exposing qemu's PID anywhere because not
every hypervisor we support has VMs as separate processes.

> 
> AFAICT the only thing printed now (@relPath) is something generated via
> qemu_driver calls (I didn't dig deep); whereas, this data is easily
> regeneratable from existing domain definition data.

Yes it is. Currently. But just look at the history of channelTargetDir,
e.g. a89f05ba8df095875f. We have to have qemuDomainSetPrivatePathsOld(),
worse we have to keep it around for the rest of libvirt's life time.
Only because nobody thought of storing channelTargerDir in runtime XML.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 27/44] Deprecate QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   | 1 -
>  src/qemu/qemu_capabilities.h   | 2 +-
>  src/qemu/qemu_command.c| 7 ---
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   | 1 -
>  tests/qemuxml2argvtest.c   | 3 +--
>  29 files changed, 2 insertions(+), 36 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 63/73] qemu: Properly reset migration params when libvirtd restarts

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:53PM +0200, Jiri Denemark wrote:

To be able to restore all migration parameters when libvirtd is
restarting during an active migration job, we need to store the original
values of all parameters (stored in priv->job.migParams) in the status
XML.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c   |   7 ++
src/qemu/qemu_migration_params.c | 145 +++
src/qemu/qemu_migration_params.h |  10 +++
3 files changed, 162 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 26/44] Deprecate QEMU_CAPS_DEVICE_SPICEVMC

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> The (now assumed) QEMU_CAPS_CHARDEV_SPICEVMC is preferred.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c |  4 --
>  src/qemu/qemu_capabilities.h |  2 +-
>  src/qemu/qemu_command.c  | 71 
> 
>  tests/qemuxml2argvdata/channel-spicevmc-old.args | 30 --
>  tests/qemuxml2argvdata/channel-spicevmc-old.xml  | 35 
>  tests/qemuxml2argvtest.c |  4 --
>  6 files changed, 25 insertions(+), 121 deletions(-)
>  delete mode 100644 tests/qemuxml2argvdata/channel-spicevmc-old.args
>  delete mode 100644 tests/qemuxml2argvdata/channel-spicevmc-old.xml

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 62/73] qemu: Set migration parameters automatically

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:52PM +0200, Jiri Denemark wrote:

Most QEMU migration parameters directly correspond to
VIR_MIGRATE_PARAM_* typed parameters and qemuMigrationParamsFromFlags
can automatically set them according to a static mapping between libvirt
and QEMU parameters.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 138 +++
1 file changed, 83 insertions(+), 55 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 25/44] Deprecate QEMU_CAPS_CHARDEV_SPICEVMC

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> @@ -1346,7 +1342,7 @@ mymain(void)
>  QEMU_CAPS_DEVICE_CIRRUS_VGA);
>  DO_TEST("channel-virtio-default",
>  QEMU_CAPS_SPICE,
> -QEMU_CAPS_CHARDEV_SPICEVMC);
> +NONE);

Don't add NONE here.

> @@ -1358,7 +1354,7 @@ mymain(void)
>  QEMU_CAPS_CCID_PASSTHRU);
>  DO_TEST("smartcard-passthrough-spicevmc",
>  QEMU_CAPS_CCID_PASSTHRU,
> -QEMU_CAPS_CHARDEV_SPICEVMC);
> +NONE);

Or here.

> @@ -1424,13 +1420,12 @@ mymain(void)
>  QEMU_CAPS_ICH9_USB_EHCI1,
>  QEMU_CAPS_USB_REDIR,
>  QEMU_CAPS_SPICE,
> -QEMU_CAPS_CHARDEV_SPICEVMC);
> +NONE);

Or here.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 24/44] Deprecate QEMU_CAPS_DRIVE_AIO

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 4b463c33cb..d39b816ec9 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -841,7 +841,6 @@ mymain(void)
>  QEMU_CAPS_KVM,
>  QEMU_CAPS_ENABLE_KVM,
>  QEMU_CAPS_PIIX3_USB_UHCI,
> -QEMU_CAPS_DRIVE_AIO,
>  QEMU_CAPS_CCID_PASSTHRU,
>  QEMU_CAPS_CHARDEV_SPICEVMC,
>  QEMU_CAPS_SPICE,
> @@ -1038,7 +1037,7 @@ mymain(void)
>  DO_TEST("disk-sata-device",
>  QEMU_CAPS_ICH9_AHCI);
>  DO_TEST("disk-aio",
> -QEMU_CAPS_DRIVE_AIO);
> +NONE);

NONE should be on the first line.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 23/44] Deprecate QEMU_CAPS_VGA_NONE

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Last use was removed by commit 0586cf98 deprecating
> QEMU_CAPS_DEVICE.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   | 1 -
>  src/qemu/qemu_capabilities.h   | 2 +-
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   | 1 -
>  tests/qemuxml2argvtest.c   | 3 +--
>  28 files changed, 2 insertions(+), 29 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 22/44] Deprecate QEMU_CAPS_SMBIOS_TYPE

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   |  1 -
>  src/qemu/qemu_capabilities.h   |  2 +-
>  src/qemu/qemu_command.c| 12 ++--
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 -
>  tests/qemuxml2argvtest.c   |  6 +++---
>  29 files changed, 6 insertions(+), 40 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu: don't retry connect() if doing FD passing

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 09:10:04AM -0400, John Ferlan wrote:
> 
> 
> On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote:
> > Since libvirt called bind() and listen() on the UNIX socket, it is
> > guaranteed that connect() will immediately succeed, if QEMU is running
> > normally. It will only fail if QEMU has closed the monitor socket by
> > mistake or if QEMU has exited, letting the kernel close it.
> > 
> > With this in mind we can remove the retry loop and timeout when
> > connecting to the QEMU monitor if we are doing FD passing. Libvirt can
> > go straight to sending the QMP greeting and will simply block waiting
> > for a reply until QEMU is ready.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/qemu/qemu_capabilities.c |  2 +-
> >  src/qemu/qemu_monitor.c  | 54 
> > ++--
> >  src/qemu/qemu_monitor.h  |  1 +
> >  src/qemu/qemu_process.c  | 27 --
> >  tests/qemumonitortestutils.c |  1 +
> >  5 files changed, 55 insertions(+), 30 deletions(-)
> > 
> 
> So just doing the monitor for now... Eventually the agent too?

Once we've attached to the monitor & got the QMP handshake done,
we know everything else has been setup correctly. So from a
functional POV there's no need to change the agent. That said
I will add another a patch, since there's no reason todo the
retry loop for the agent - even before FD passing, we should
never have been retrying AFAICT, since we should be guaranteed
that it exists at the time we connect.

> How about having a news.xml patch - is this a newsworthy change?

Its mostly invisible to the user I would hope

> 
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 57c06c7c15..9950461810 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -1771,7 +1771,7 @@ qemuProcessInitMonitor(virQEMUDriverPtr driver,
> >  
> >  static int
> >  qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int 
> > asyncJob,
> > -   qemuDomainLogContextPtr logCtxt)
> > +   bool retry, qemuDomainLogContextPtr logCtxt)
> >  {
> >  qemuDomainObjPrivatePtr priv = vm->privateData;
> >  qemuMonitorPtr mon = NULL;
> > @@ -1799,6 +1799,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> > virDomainObjPtr vm, int asyncJob,
> >  mon = qemuMonitorOpen(vm,
> >priv->monConfig,
> >priv->monJSON,
> > +  retry,
> >timeout,
> >,
> >driver);
> > @@ -2174,17 +2175,23 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
> >  {
> >  int ret = -1;
> >  virHashTablePtr info = NULL;
> > -qemuDomainObjPrivatePtr priv;
> > +qemuDomainObjPrivatePtr priv = vm->privateData;
> > +bool retry = true;
> 
> This is also called from qemuProcessAttach where it wouldn't seem
> there'd be necessarily be a chardev on the command line with the
> pre-opened fd, but then again since it's being used for an already
> running qemu instance, that would "I hope" work properly...
> 
> With that assumption in place,

Yes, exactly - when attaching to an existing QEMU, we only want to
try once and then abort, because either the socket exists, or the
QEMU we're attaching to has just quit. Retrying was never right
in that scenario.

> 
> Reviewed-by: John Ferlan 
> 
> John
> 
> > +
> > +if (priv->qemuCaps &&
> > +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS))
> > +retry = false;
> >  
> > -VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name);
> > -if (qemuConnectMonitor(driver, vm, asyncJob, logCtxt) < 0)
> > +VIR_DEBUG("Connect monitor to vm=%p name='%s' retry=%d",
> > +  vm, vm->def->name, retry);
> > +
> > +if (qemuConnectMonitor(driver, vm, asyncJob, retry, logCtxt) < 0)
> >  goto cleanup;
> >  
> >  /* Try to get the pty path mappings again via the monitor. This is 
> > much more
> >   * reliable if it's available.
> >   * Note that the monitor itself can be on a pty, so we still need to 
> > try the
> >   * log output method. */
> > -priv = vm->privateData;
> >  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> >  goto cleanup;
> >  ret = qemuMonitorGetChardevInfo(priv->mon, );
> 
> [...]
> 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 21/44] Deprecate QEMU_CAPS_NAME_PROCESS

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   | 1 -
>  src/qemu/qemu_capabilities.h   | 2 +-
>  src/qemu/qemu_command.c| 3 +--
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   | 1 -
>  28 files changed, 2 insertions(+), 29 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 20/44] Deprecate QEMU_CAPS_FSDEV

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   |  1 -
>  src/qemu/qemu_capabilities.h   |  2 +-
>  src/qemu/qemu_command.c|  6 --
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 -
>  tests/qemuxml2argvtest.c   | 10 +-
>  tests/qemuxml2xmltest.c|  6 --
>  30 files changed, 2 insertions(+), 48 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 19/44] Deprecate QEMU_CAPS_BOOT_MENU

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 11f4b86610..7fce1e9b59 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -766,24 +766,21 @@ mymain(void)
>  QEMU_CAPS_ICH9_AHCI);
>  DO_TEST("bootindex-floppy-q35",
>  QEMU_CAPS_DEVICE_IOH3420,
> -QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_BOOT_MENU,
> +QEMU_CAPS_ICH9_AHCI,
>  QEMU_CAPS_BOOTINDEX);
> -DO_TEST("boot-multi", QEMU_CAPS_BOOT_MENU);
> +DO_TEST("boot-multi", NONE);
>  DO_TEST("boot-menu-enable",
> -QEMU_CAPS_BOOT_MENU);
> +NONE);

NONE goes on the first line.

>  DO_TEST("boot-menu-enable-bootindex",
> -QEMU_CAPS_BOOT_MENU,
>  QEMU_CAPS_BOOTINDEX);
>  DO_TEST("boot-menu-enable-with-timeout",
> -QEMU_CAPS_BOOT_MENU,
>  QEMU_CAPS_SPLASH_TIMEOUT);
> -DO_TEST_FAILURE("boot-menu-enable-with-timeout", QEMU_CAPS_BOOT_MENU);
> +DO_TEST_FAILURE("boot-menu-enable-with-timeout", NONE);
>  DO_TEST_PARSE_ERROR("boot-menu-enable-with-timeout-invalid", NONE);
> -DO_TEST("boot-menu-disable", QEMU_CAPS_BOOT_MENU);
> +DO_TEST("boot-menu-disable", NONE);
>  DO_TEST("boot-menu-disable-drive",
> -QEMU_CAPS_BOOT_MENU);
> +NONE);

Here too.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 18/44] Deprecate QEMU_CAPS_NODEFCONFIG

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> @@ -1349,156 +1305,133 @@ mymain(void)
>  driver.config->chardevTLS = 0;
>  VIR_FREE(driver.config->chardevTLSx509certdir);
>  DO_TEST("serial-many-chardev",
> -QEMU_CAPS_DEVICE_ISA_SERIAL,
> -QEMU_CAPS_NODEFCONFIG);
> -DO_TEST("parallel-tcp-chardev",
> -QEMU_CAPS_NODEFCONFIG);
> +QEMU_CAPS_DEVICE_ISA_SERIAL);
> +DO_TEST("parallel-tcp-chardev", NONE);
>  DO_TEST("parallel-parport-chardev",
> -QEMU_CAPS_NODEFCONFIG);
> +NONE);

NONE should be on the same line as the test name in this case.
There are a few more instances of the same issue below.

>  DO_TEST("console-compat-chardev",
>  QEMU_CAPS_DEVICE_ISA_SERIAL,
> -QEMU_CAPS_NODEFCONFIG);
> +NONE);

I guess technically this will work, but it's definitely not what
you were aiming for, is it? Again, more instances below.

> @@ -1783,14 +1698,10 @@ mymain(void)
>  QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
>  
>  DO_TEST("multifunction-pci-device",
> -QEMU_CAPS_NODEFCONFIG,
>  QEMU_CAPS_SCSI_LSI);
>  
> -DO_TEST("monitor-json",
> -QEMU_CAPS_NODEFCONFIG);
> -DO_TEST("no-shutdown",
> -QEMU_CAPS_NODEFCONFIG,
> -QEMU_CAPS_NO_SHUTDOWN);
> +DO_TEST("monitor-json", NONE);
> +DO_TEST("no-shutdown", QEMU_CAPS_NO_SHUTDOWN);

Here the NO_SHUTDOWN capability should have stayed on its own line.

I'm attaching a patch that fixes all the issues I could find,
because that's faster than pointing them all out :)

With that squashed in

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / VirtualizationFrom c2c4f6f937a450685dc53ea1859c8f11f87f13b5 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani 
Date: Mon, 16 Apr 2018 15:03:23 +0200
Subject: [PATCH] fixup

Signed-off-by: Andrea Bolognani 
---
 tests/qemuxml2argvtest.c | 93 ++--
 tests/qemuxml2xmltest.c  | 15 +++-
 2 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f5edb0eaba..5ca8da6230 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1305,31 +1305,21 @@ mymain(void)
 DO_TEST("serial-many-chardev",
 QEMU_CAPS_DEVICE_ISA_SERIAL);
 DO_TEST("parallel-tcp-chardev", NONE);
-DO_TEST("parallel-parport-chardev",
-NONE);
+DO_TEST("parallel-parport-chardev", NONE);
 DO_TEST("console-compat-chardev",
-QEMU_CAPS_DEVICE_ISA_SERIAL,
-NONE);
+QEMU_CAPS_DEVICE_ISA_SERIAL);
 DO_TEST("pci-serial-dev-chardev",
 QEMU_CAPS_DEVICE_PCI_SERIAL);
 
-DO_TEST("channel-guestfwd",
-NONE);
-DO_TEST("channel-virtio",
-NONE);
-DO_TEST("channel-virtio-state",
-NONE);
-DO_TEST("channel-virtio-auto",
-NONE);
-DO_TEST("channel-virtio-autoassign",
-NONE);
-DO_TEST("channel-virtio-autoadd",
-NONE);
-DO_TEST("console-virtio",
-NONE);
+DO_TEST("channel-guestfwd", NONE);
+DO_TEST("channel-virtio", NONE);
+DO_TEST("channel-virtio-state", NONE);
+DO_TEST("channel-virtio-auto", NONE);
+DO_TEST("channel-virtio-autoassign", NONE);
+DO_TEST("channel-virtio-autoadd", NONE);
+DO_TEST("console-virtio", NONE);
 DO_TEST("console-virtio-many",
-QEMU_CAPS_DEVICE_ISA_SERIAL,
-NONE);
+QEMU_CAPS_DEVICE_ISA_SERIAL);
 DO_TEST("console-virtio-s390",
 QEMU_CAPS_BOOTINDEX,
 QEMU_CAPS_VIRTIO_S390);
@@ -1351,8 +1341,7 @@ mymain(void)
 DO_TEST("channel-virtio-default",
 QEMU_CAPS_SPICE,
 QEMU_CAPS_CHARDEV_SPICEVMC);
-DO_TEST("channel-virtio-unix",
-NONE);
+DO_TEST("channel-virtio-unix", NONE);
 
 DO_TEST("smartcard-host",
 QEMU_CAPS_CCID_EMULATED);
@@ -1377,15 +1366,12 @@ mymain(void)
 DO_TEST_PARSE_ERROR("chardev-reconnect-generated-path",
 QEMU_CAPS_CHARDEV_RECONNECT);
 
-DO_TEST("usb-controller",
-NONE);
+DO_TEST("usb-controller", NONE);
 DO_TEST("usb-piix3-controller",
-QEMU_CAPS_PIIX3_USB_UHCI,
-NONE);
+QEMU_CAPS_PIIX3_USB_UHCI);
 DO_TEST("usb-ich9-ehci-addr",
 QEMU_CAPS_ICH9_USB_EHCI1);
-DO_TEST("input-usbmouse-addr",
-NONE);
+DO_TEST("input-usbmouse-addr", NONE);
 DO_TEST("usb-ich9-companion",
 QEMU_CAPS_ICH9_USB_EHCI1);
 DO_TEST_PARSE_ERROR("usb-ich9-no-companion",
@@ -1394,35 +1380,25 @@ mymain(void)
 QEMU_CAPS_ICH9_USB_EHCI1,
 QEMU_CAPS_USB_HUB);
 DO_TEST("usb-hub",
-QEMU_CAPS_USB_HUB,
-NONE);
+QEMU_CAPS_USB_HUB);
 

Re: [libvirt] [PATCH 8/9] virobject: Check if @parent is the first member in class

[Daniel P . Berrangé]

On Mon, Apr 16, 2018 at 02:30:40PM +0200, Erik Skultety wrote:
> On Fri, Apr 13, 2018 at 04:47:15PM +0200, Michal Privoznik wrote:
> > Our virObject code relies heavily on the fact that the first
> > member of the class struct is type of virObject (or some
> > derivation of if). Let's check for that.
> 
> If a class is missing 'parent' memeber, it's a bug in the definition of the
> struct/class, therefore there should be a static assertion rather than a
> runtime check.

Agreed, my suggestion was for a static assert too.

> 
> >
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/util/virobject.c | 31 +--
> >  src/util/virobject.h |  5 -
> >  2 files changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/util/virobject.c b/src/util/virobject.c
> > index c5a98d21cc..e184f5349e 100644
> > --- a/src/util/virobject.c
> > +++ b/src/util/virobject.c
> > @@ -77,6 +77,7 @@ virObjectOnceInit(void)
> >  {
> >  if (!(virObjectClass = virClassNew(NULL,
> > "virObject",
> > +   0,
> > sizeof(virObject),
> > NULL)))
> 
> Also, I don't like this extra parameter, which really shouldn't be needed; you
> created a macro which hides this parameter, but that doesn't mean that
> design-wise it makes sense to have it there, think of it as a constructor, you
> don't pass a constructor an offset of the class' member, because it shouldn't
> have need for it, but you do, solely for the purpose of checking whether we 
> have
> a particular member in place.
> So, to start a discussion about this (I also think Dan posted something 
> related
> to this recently, but I don't seem to be able to find it in the archives - do 
> I
> even archive?!!!), I came up with my first compile-time hack ever, it seems to
> work like expected, but I'd like to hear your opinions both the macro itself
> and the approach we're going to take, so here's my replacement patch:

https://www.redhat.com/archives/libvir-list/2018-April/msg00984.html

I had suggested something in the virObjectNew function:

  #define virObjectNew(typ) \
  (typ *)(&((typ *)virObjectNewImpl(typ # Class)).parent)

catching it with virClassNew works fine too, as it would be a compile
time check too

> diff --git a/src/util/virobject.h b/src/util/virobject.h
> index 92dd51239..2a973d401 100644
> --- a/src/util/virobject.h
> +++ b/src/util/virobject.h
> @@ -75,8 +75,12 @@ virClassPtr virClassForObjectRWLockable(void);
>  #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
>  # endif
> 
> +# define VIR_CLASS_HAS_PARENT(name) \
> +!sizeof(char[0-offsetof(name, parent)])
> +
>  # define VIR_CLASS_NEW(prnt, name) \
> -virClassNew(prnt, #name, sizeof(name), name##Dispose)
> +VIR_CLASS_HAS_PARENT(name) ? \
> +virClassNew(prnt, #name, sizeof(name), name##Dispose) : NULL

So we're relying on the fact the the ': NULL" will never execute
because VIR_CLASS_HAS_PARENT will trigger a compile time error.

> 
> Notes:
>  - I suppose mingw would handle this hack the same way it handles
>VIR_TYPEMATCH, IOW it works...
> 
>  - it also doesn't need to be a ternary, I suggested extending VIR_CLASS_NEW 
> to
>do the complete assignment in [Patch 7/9], like this:
> 
> # define VIR_CLASS_NEW(prnt, name) \
> if (!(name##Class) = virClassNew(prnt, #name, sizeof(name), 
> name##Dispose))
> return -1;

This has the added benefit of enforcing class variable naming scheme
which removes another source of developer error, and is in keeping
with VIR_ALLOC() style.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 17/44] Deprecate QEMU_CAPS_VHOST_NET

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   | 1 -
>  src/qemu/qemu_capabilities.h   | 2 +-
>  src/qemu/qemu_command.c| 3 +--
>  src/qemu/qemu_hotplug.c| 9 +++--
>  src/qemu/qemu_interface.c  | 4 +---
>  src/qemu/qemu_interface.h  | 1 -
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  | 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   | 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   | 1 -
>  31 files changed, 6 insertions(+), 39 deletions(-)

Looks like we have zero test suite coverage for the vhost-net
feature. Neat.

[...]
> @@ -645,8 +644,7 @@ qemuInterfaceOpenVhostNet(virDomainDefPtr def,
>  /* If qemu doesn't support vhost-net mode (including the -netdev command
>   * option), don't try to open the device.
>   */
> -if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOST_NET) &&
> -  qemuDomainSupportsNicdev(def, net))) {
> +if (!(qemuDomainSupportsNicdev(def, net))) {
>  if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("vhost-net is not supported with "

The parentheses around the call to qemuDomainSupportsNicdev() are
unnecessary now, please get rid of them.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 8/9] virobject: Check if @parent is the first member in class

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:15PM +0200, Michal Privoznik wrote:
> Our virObject code relies heavily on the fact that the first
> member of the class struct is type of virObject (or some
> derivation of if). Let's check for that.

If a class is missing 'parent' memeber, it's a bug in the definition of the
struct/class, therefore there should be a static assertion rather than a
runtime check.

>
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virobject.c | 31 +--
>  src/util/virobject.h |  5 -
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index c5a98d21cc..e184f5349e 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -77,6 +77,7 @@ virObjectOnceInit(void)
>  {
>  if (!(virObjectClass = virClassNew(NULL,
> "virObject",
> +   0,
> sizeof(virObject),
> NULL)))

Also, I don't like this extra parameter, which really shouldn't be needed; you
created a macro which hides this parameter, but that doesn't mean that
design-wise it makes sense to have it there, think of it as a constructor, you
don't pass a constructor an offset of the class' member, because it shouldn't
have need for it, but you do, solely for the purpose of checking whether we have
a particular member in place.
So, to start a discussion about this (I also think Dan posted something related
to this recently, but I don't seem to be able to find it in the archives - do I
even archive?!!!), I came up with my first compile-time hack ever, it seems to
work like expected, but I'd like to hear your opinions both the macro itself
and the approach we're going to take, so here's my replacement patch:

diff --git a/src/util/virobject.h b/src/util/virobject.h
index 92dd51239..2a973d401 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -75,8 +75,12 @@ virClassPtr virClassForObjectRWLockable(void);
 #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
 # endif

+# define VIR_CLASS_HAS_PARENT(name) \
+!sizeof(char[0-offsetof(name, parent)])
+
 # define VIR_CLASS_NEW(prnt, name) \
-virClassNew(prnt, #name, sizeof(name), name##Dispose)
+VIR_CLASS_HAS_PARENT(name) ? \
+virClassNew(prnt, #name, sizeof(name), name##Dispose) : NULL

Notes:
 - I suppose mingw would handle this hack the same way it handles
   VIR_TYPEMATCH, IOW it works...

 - it also doesn't need to be a ternary, I suggested extending VIR_CLASS_NEW to
   do the complete assignment in [Patch 7/9], like this:

# define VIR_CLASS_NEW(prnt, name) \
if (!(name##Class) = virClassNew(prnt, #name, sizeof(name), name##Dispose))
return -1;

Regards,
Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 4/4] qemu: deny privilege elevation and spawn in seccomp

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 04:49:42PM +0200, Ján Tomko wrote:
> If QEMU uses a seccomp blacklist (since 2.11), -sandbox on
> no longer tries to whitelist all the calls, but uses sets
> of blacklists:
> default (always blacklisted with -sandbox on)
> obsolete (defaults to deny)
> elevateprivileges (setuid & co, default: allow)
> spawn (fork & execve, default: allow)
> resourcecontrol (setaffinity, setscheduler, default: allow)
> 
> If these are supported, default to sandbox with all four
> categories blacklisted.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1492597
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu.conf  |  7 +++---
>  src/qemu/qemu_command.c | 10 +
>  tests/qemuxml2argvdata/minimal-sandbox.args | 29 
>  tests/qemuxml2argvdata/minimal-sandbox.xml  | 34 
> +
>  tests/qemuxml2argvtest.c| 11 ++
>  5 files changed, 88 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.args
>  create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.xml
> 
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 07eab7eff..740129cf5 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -669,9 +669,10 @@
>  
>  
>  
> -# Use seccomp syscall whitelisting in QEMU.
> -# 1 = on, 0 = off, -1 = use QEMU default
> -# Defaults to -1.
> +# Use seccomp syscall sandbox in QEMU.
> +# 1 = on, 0 = off, -1 = use the default
> +# For QEMUs using a whitelist, the default (-1) is off.
> +# For QEMUs using a blacklist, the default (-1) is on.

I'd suggest rewriting this a bit:

 # 1 == seccomp enabled, 0 == seccomp disabled
 # 
 # If it is unset (or -1), then seccomp will be enabled
 # only if QEMU >= 2.11.0 is detected, otherwise it is
 # left disabled. This ensures the default config gets
 # protection for new QEMU using the blacklist approach.

>  #
>  #seccomp_sandbox = 1
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ba279e640..fa5906d0b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9987,6 +9987,16 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
>  return 0;
>  }
>  
> +/* Use blacklist by default if supported */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) {
> +virCommandAddArgList(cmd, "-sandbox",
> + "on,obsolete=deny,elevateprivileges=deny,"
> + "spawn=deny,resourcecontrol=deny",
> + NULL);
> +return 0;
> +}
> +
> +/* Seccomp whitelist is opt-in */
>  if (cfg->seccompSandbox > 0)
>  virCommandAddArgList(cmd, "-sandbox", "on", NULL);

Reviewed-by: Daniel P. Berrangé 


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 3/4] Refactor qemuBuildSeccompSandboxCommandLine

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 04:49:41PM +0200, Ján Tomko wrote:
> Exit early if possible to simplify the logic.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dfeba54ee..ba279e640 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9974,16 +9974,22 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> virQEMUCapsPtr qemuCaps)
>  {
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
> -if (cfg->seccompSandbox == 0)
> -virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> -else if (cfg->seccompSandbox > 0)
> -virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> -} else if (cfg->seccompSandbox > 0) {
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX) &&
> +cfg->seccompSandbox > 0) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("QEMU does not support seccomp sandboxes"));
>  return -1;
>  }
> +
> +if (cfg->seccompSandbox == 0) {
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX))
> +virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> +return 0;
> +}
> +
> +if (cfg->seccompSandbox > 0)
> +virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> +
>  return 0;
>  
>  }
> -- 
> 2.16.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 2/4] Introduce qemuBuildSeccompSandboxCommandLine

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 04:49:40PM +0200, Ján Tomko wrote:
> Move the building of -sandbox command line into a separate function.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 30 +-
>  1 file changed, 21 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 514c3ab2e..dfeba54ee 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9969,6 +9969,26 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
> +   virQEMUDriverConfigPtr cfg,
> +   virQEMUCapsPtr qemuCaps)
> +{
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
> +if (cfg->seccompSandbox == 0)
> +virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> +else if (cfg->seccompSandbox > 0)
> +virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> +} else if (cfg->seccompSandbox > 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("QEMU does not support seccomp sandboxes"));
> +return -1;
> +}
> +return 0;
> +
> +}
> +
> +
>  /*
>   * Constructs a argv suitable for launching qemu with config defined
>   * for a given virtual machine.
> @@ -10206,16 +10226,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>   ? qemucmd->env_value[i] : "");
>  }
>  
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
> -if (cfg->seccompSandbox == 0)
> -virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> -else if (cfg->seccompSandbox > 0)
> -virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> -} else if (cfg->seccompSandbox > 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("QEMU does not support seccomp sandboxes"));
> +if (qemuBuildSeccompSandboxCommandLine(cmd, cfg, qemuCaps) < 0)
>  goto error;
> -}
>  
>  if (qemuBuildPanicCommandLine(cmd, def, qemuCaps) < 0)
>  goto error;
> -- 
> 2.16.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 1/4] Introduce QEMU_CAPS_SECCOMP_BLACKLIST

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 04:49:39PM +0200, Ján Tomko wrote:
> QEMU commit 1bd6152 changed the default behavior from whitelist
> to blacklist and introduced a few sets of system calls.
> 
> Use the 'elevateprivileges' parameter of -sandbox as a witness
> of this change.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1492597
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c   | 2 ++
>  src/qemu/qemu_capabilities.h   | 1 +
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 +
>  7 files changed, 8 insertions(+)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 35905e993..729e29e20 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"virtio-tablet-ccw",
>"qcow2-luks",
>"pcie-pci-bridge",
> +  "seccomp-blacklist",
>  );
>  
>  
> @@ -3214,6 +3215,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>  { "machine", "loadparm", QEMU_CAPS_LOADPARM },
>  { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS },
>  { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT },
> +{ "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST },
>  };
>  
>  static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index bec28cae9..d88102f34 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -452,6 +452,7 @@ typedef enum {
>  QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */
>  QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */
>  QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */
> +QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml 
> b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
> index cbd645ae9..3861666e5 100644
> --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
> @@ -151,6 +151,7 @@
>
>
>
> +  
>2011000
>0
>342058
> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml 
> b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
> index 66629ff5b..39238a9b6 100644
> --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
> @@ -189,6 +189,7 @@
>
>
>
> +  
>2011090
>0
>342346
> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml 
> b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
> index 1122d6408..6bf293b9d 100644
> --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
> @@ -186,6 +186,7 @@
>
>
>
> +  
>2011090
>0
>419215
> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml 
> b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
> index 191b1e0e3..b77aec9c9 100644
> --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
> @@ -151,6 +151,7 @@
>
>
>
> +  
>2011090
>0
>0
> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml 
> b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
> index 4ed2e1ea9..1bb825c9b 100644
> --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
> @@ -227,6 +227,7 @@
>
>
>
> +  
>2011090
>0
>390060
> -- 
> 2.16.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] openvz: Clean up openvzDomainGetHostname

[Ján Tomko]

On Wed, Apr 11, 2018 at 12:57:08PM -0400, John Ferlan wrote:

Remove the unnecessary goto error followed by goto cleanup
processing.

Signed-off-by: John Ferlan 
---
src/openvz/openvz_driver.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] openvz: Remove unnecessary Unref in openvzLoadDomains

[Ján Tomko]

On Wed, Apr 11, 2018 at 12:57:07PM -0400, John Ferlan wrote:

Since there is no way to get to cleanup without dom being NULL,
this is a unnecessary Unref.



Actually, if we could get there it would be a bug, because
virDomainObjListAdd does not give us an extra reference, it just leaves
the domain locked.


Signed-off-by: John Ferlan 
---
src/openvz/openvz_conf.c | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[Erik Skultety]

On Mon, Apr 16, 2018 at 07:53:42AM -0400, John Ferlan wrote:
>
>
> On 04/13/2018 10:47 AM, Michal Privoznik wrote:
> > In next patches this name will be needed for a different memeber.
>
> s/memeber/member
>
> > Also, it makes sense to rename the variable because it does not
> > contain reference to parent device, just its name.
> >
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/conf/virnodedeviceobj.c  | 2 +-
> >  src/datatypes.c  | 2 +-
> >  src/datatypes.h  | 2 +-
> >  src/libvirt-nodedev.c| 6 +++---
> >  src/node_device/node_device_driver.c | 4 ++--
> >  src/test/test_driver.c   | 6 +++---
> >  6 files changed, 11 insertions(+), 11 deletions(-)
> >
>
> [...]
>
> FWIW: Not sure about changing the accessor's name too - that would mean
> the API name doesn't match (virNodeDeviceGetParent which could lead to
> other problems with automatic code generation).

Oh, is that so?! I missed this fact, sorry for the noise then. Nevertheless,
it's sad we've such a mistake.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[John Ferlan]

On 04/13/2018 10:47 AM, Michal Privoznik wrote:
> In next patches this name will be needed for a different memeber.

s/memeber/member

> Also, it makes sense to rename the variable because it does not
> contain reference to parent device, just its name.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/virnodedeviceobj.c  | 2 +-
>  src/datatypes.c  | 2 +-
>  src/datatypes.h  | 2 +-
>  src/libvirt-nodedev.c| 6 +++---
>  src/node_device/node_device_driver.c | 4 ++--
>  src/test/test_driver.c   | 6 +++---
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 

[...]

FWIW: Not sure about changing the accessor's name too - that would mean
the API name doesn't match (virNodeDeviceGetParent which could lead to
other problems with automatic code generation).

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 16/44] Deprecate QEMU_CAPS_RTC_TD_HACK

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 0.12.0.
> 
> Deprecated by QEMU commit 1ed2fc1 included in 0.12.0.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.h |  2 +-
>  src/qemu/qemu_command.c  | 21 +
>  2 files changed, 2 insertions(+), 21 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 15/44] Deprecate QEMU_CAPS_RTC

[Andrea Bolognani]

On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:
> Implied by QEMU >= 1.2.0.
> 
> Signed-off-by: Ján Tomko 

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] lxc: Clean up /.oldroot

[Radostin Stoyanov]

On 16/04/18 10:33, Daniel P. Berrangé wrote:
> On Sun, Apr 15, 2018 at 04:30:11PM +0100, Radostin Stoyanov wrote:
>> Remove the /.oldroot directory after it has been unmounted (at the end
>> of lxcContainerSetupPivotRoot). Ignore errors silently.
>>
>> Signed-off-by: Radostin Stoyanov 
>> ---
>>  src/lxc/lxc_container.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
>> index 665b93a0a..dd4e38703 100644
>> --- a/src/lxc/lxc_container.c
>> +++ b/src/lxc/lxc_container.c
>> @@ -1811,6 +1811,9 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr 
>> vmDef,
>>  if (lxcContainerUnmountSubtree("/.oldroot", true) < 0)
>>  goto cleanup;
>>  
>> +if (virFileRemove("/.oldroot", 0, 0) < 0)
>> +VIR_DEBUG("Failed to remove /.oldroot after start");
>> +
> I think this introduces a race condition. There can be two containers
> with the same root filesystem. If we start both at the same time, then
> this deletion of /.oldroot can cause the other contanier to fail to
> start if it saw that /.oldroot already existed & it thus tried to skip
> mkdir.
Thank you for the review, I hadn't thought about this case.
> Leaving the empty directory is harmless IMHO
I agree that leaving the empty directory is harmless.

Regards,
Radostin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH] maint: rename test directory to tests

[Pavel Hrdina]

On Mon, Apr 16, 2018 at 01:10:22PM +0200, Pavel Hrdina wrote:
> Our ci.centos.org expects the project uses 'tests' name.
> 
> Signed-off-by: Pavel Hrdina 
> ---

I forgot to mention that it's pushed.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/8] qemu: introduce vfio-ccw capability

[Shalini Chellathurai Saroja]

On 04/12/2018 10:44 AM, Cornelia Huck wrote:

On Wed, 11 Apr 2018 17:49:53 +0200
Shalini Chellathurai Saroja  wrote:


Let us introduce the capability vfio-ccw for supporting the basic
channel I/O passthrough, which have been introduced in QEMU 2.10. The
current focus is to support dasd-eckd (cu_type/dev_type = 0x3990/0x3390)
as the target device.

Let us also introduce the capability QEMU_CAPS_CCW_CSSID_UNRESTRICTED
for virtual-css-bridge. This capability is based on the
cssid-unrestricted property which exists if QEMU no longer enforces
cssid restrictions based on ccw device types.

Vfio-ccw capability is dependent on the hidden virtual-css-bridge, so
that we are able to probe for the cssid-unrestriced property.

You depend on the unrestricted cssids and do not support the old,
deprecated squash-mcss approach? Makes sense.

I'm not familiar with libvirt conventions, but does it make sense to
add a comment _why_ vfio-ccw depends on unrestricted cssids (i.e., to
make sure the devices are visible to non-mcss-e enabled guests?)


ok, I will add the comment, thanks.


Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
  src/qemu/qemu_capabilities.c   | 14 ++
  src/qemu/qemu_capabilities.h   |  4 +++
  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 28 ---
  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  2 +-
  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 28 ---
  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  2 +-
  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 31 --
  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  3 +++
  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 24 -
  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  2 +-
  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 28 ---
  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  2 +-
  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 28 ---
  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  2 +-
  14 files changed, 142 insertions(+), 56 deletions(-)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH] maint: rename test directory to tests

[Pavel Hrdina]

Our ci.centos.org expects the project uses 'tests' name.

Signed-off-by: Pavel Hrdina 
---
 .travis.yml | 2 +-
 Makefile.am | 2 +-
 configure.ac| 2 +-
 {test => tests}/Makefile.am | 0
 {test => tests}/conftest.py | 0
 {test => tests}/libvirttest.py  | 0
 {test => tests}/test_connect.py | 0
 {test => tests}/test_domain.py  | 0
 {test => tests}/test_network.py | 0
 {test => tests}/travis-run  | 0
 10 files changed, 3 insertions(+), 3 deletions(-)
 rename {test => tests}/Makefile.am (100%)
 rename {test => tests}/conftest.py (100%)
 rename {test => tests}/libvirttest.py (100%)
 rename {test => tests}/test_connect.py (100%)
 rename {test => tests}/test_domain.py (100%)
 rename {test => tests}/test_network.py (100%)
 rename {test => tests}/travis-run (100%)

diff --git a/.travis.yml b/.travis.yml
index 71c2e9c..d60eaf2 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,7 +1,7 @@
 dist: trusty
 language: c
 sudo: true
-script: test/travis-run
+script: tests/travis-run
 env:
  - ARCH=amd64
  - ARCH=i386
diff --git a/Makefile.am b/Makefile.am
index a890ff1..16f57b2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,5 +1,5 @@
 
-SUBDIRS = data src test
+SUBDIRS = data src tests
 
 ACLOCAL_AMFLAGS = -I m4
 
diff --git a/configure.ac b/configure.ac
index ea2c330..6f166f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -75,5 +75,5 @@ AC_CONFIG_FILES([run],
 AC_OUTPUT(Makefile
   data/Makefile
   src/Makefile
-  test/Makefile
+  tests/Makefile
   libvirt-dbus.spec)
diff --git a/test/Makefile.am b/tests/Makefile.am
similarity index 100%
rename from test/Makefile.am
rename to tests/Makefile.am
diff --git a/test/conftest.py b/tests/conftest.py
similarity index 100%
rename from test/conftest.py
rename to tests/conftest.py
diff --git a/test/libvirttest.py b/tests/libvirttest.py
similarity index 100%
rename from test/libvirttest.py
rename to tests/libvirttest.py
diff --git a/test/test_connect.py b/tests/test_connect.py
similarity index 100%
rename from test/test_connect.py
rename to tests/test_connect.py
diff --git a/test/test_domain.py b/tests/test_domain.py
similarity index 100%
rename from test/test_domain.py
rename to tests/test_domain.py
diff --git a/test/test_network.py b/tests/test_network.py
similarity index 100%
rename from test/test_network.py
rename to tests/test_network.py
diff --git a/test/travis-run b/tests/travis-run
similarity index 100%
rename from test/travis-run
rename to tests/travis-run
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Vladimir Sementsov-Ogievskiy]

13.04.2018 23:02, John Snow wrote:


On 04/12/2018 10:08 AM, Vladimir Sementsov-Ogievskiy wrote:

It's not easier, as we'll have to implement either separate of bitmaps
concept of checkpoints, which will be based on bitmaps, and we'll have
to negotiate and implement storing these objects to qcow2 and migrate
them. Or we'll go through proposed by Kevin (If I remember correctly)
way of adding something like "backing" or "parent" pointer to
BdrvDirtyBitmap, and anyway store to qcow2, migrate and expose qapi for
them. The other (more hard way) is move to multi-bit bitmaps (like in
vmware), where for each granularity-chunk we store a number,
representing "before which checkpoint was the latest change of this
chunk", and the same, qapi+qcow2+migration.

It's all not easier than call several simple qmp commands.

OK, I just wanted to explore the option before we settled on using the
name as metadata.

What are the downsides to actually including a predecessor/successor*
pointer in QEMU?


the problem is the following: we want checkpoints, and it is bad to 
implement them through additional mechanism, which is definitely not 
"checkpoints".


With checkpoints in qemu:
 - for user checkpoints are unrelated to dirty bitmaps, they are 
managed separately, it's safe
 - clean api realisation in libvirt, to remove a checkpoint libvirt 
will call qmp block-checkpoint-remove


With additional pointer in BdrvDirtyBitmap:
 - checkpoint-related bitmaps share same namespace with other bitmaps
 - user still can remove bitmap from the chain, without corresponding 
merge, which breaks the whole thing
 - we'll need to implement api like for block layer : bitmap-commit, 
bitmap-pull, etc.. (or just leave my merge), but it's all not what we 
want, not checkpoints.


So my point is: if we are going to implement something complicated, 
let's implement entirely what we want, not a semi-solution. Otherwise, 
implement a minimal and simple thing, to just make it all work (my 
current solution).


So, if you agree with me, that true way is checkpoints api for qemu, 
there things, which we need to implement:


1. multi-bit dirty bitmaps (if I remember correctly, such thing is done 
in vmware), that is:


   For each data chunk we have not one bit, but several, and store a 
number, which refer to last checkpoint, after which there were changes 
in this area. So, if we want to get blocks, changed from snapshot N up 
to current time, we just take all blocks, for which this number is >= N. 
It is more memory-efficient, then storing several dirty bitmaps.


On the other hand, several linked dirty bitmaps have the other 
advantage: we can store in RAM only the last, the active one, and other 
on disk. And load them only on demand, when we need to merge. So, it 
looks like true way is combination of multi- and one- bit dirty bitmaps, 
with ability to load/store them to disk dinamically. So we need


2. Some link(s) in BdrvDirtyBitmap, to implement relations. May be, it's 
better to store two links to checkpoints, for which the bitmap defines 
the difference. (or to several checkpoints, if it is a multi-bit bitmap)


3. Checkpoints objects, with separate management, backed by dirty 
bitmaps (one- or multi-). These bitmaps should not be directly 
accessible by user, but user should have a possibility to setup a 
strategy (one- or multi- bit, or their combinations, keep all in RAM, or 
keep inactive on disk and active in RAM, etc).


4. All these things should be stored to qcow2, all should successfully 
migrate, and we also need to thing about NBD exporting (however, looks 
like NBD protocol is flexible enough to do it)


===

Also, we need to understand, what are user cases for this all.

case 1. Incremental restore to some point in past: If we know, which 
blocks are modified since this point, we can copy only these blocks from 
backup. But, it's obvious that this information can be extracted from 
backup itself (we should know, which blocks was actually backed up). So, 
I'm not sure that this all worth doing.


case 2. several inc-backup chains to different backup storages with 
different timesheets. Actually, we support it by just several active 
dirty bitmaps. But it looks inefficient:


What is the reason to maintain several active dirty bitmaps, which are 
used seldom? They eat RAM and CPU time on each write. It looks better to 
have only one active bitmap, and several disabled, which we can store in 
the disk, not in RAM. And this leads us to checkpoints.. Checkpoints are 
more natural for users to make backups, then dirty bitmaps. And 
checkpoints give a way to improve ram and cpu usage.


As a first step the following may be done:

Add two string fields to BdrvDirtyBitmap:

  checkpoint-from
  checkpoint-to

Which defines checkpoint names. For such bitmaps name field should be 
zero. Add these fields to qcow2 bitmap representation and to migration 
protocol. Add checkpoint api (create/remove/nbd export). Deprecate 
bitmap api (move to 

Re: [libvirt] [PATCH 6/9] Introduce virNetSASLContextDispose

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:13PM +0200, Michal Privoznik wrote:
> Strictly speaking this is not needed right now. However, next
> commits will require dispose function to exist.
>
> Signed-off-by: Michal Privoznik 
> ---

...

>  src/rpc/virnetsaslcontext.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> +void virNetSASLContextDispose(void *obj ATTRIBUTE_UNUSED)
> +{
> +/* nada */

One more thing, either leave the body empty, or replace the commentary with a
plain "return" call.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 9/9] cfg.mk: Introduce syntax-check rule to prefer VIR_CLASS_NEW

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:16PM +0200, Michal Privoznik wrote:
> Now that we have macro that does some checks lets forbid raw
> usage of virClassNew() in favor of VIR_CLASS_NEW().
>
> Signed-off-by: Michal Privoznik 
> ---
>  cfg.mk | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/cfg.mk b/cfg.mk
> index 4078bc2c63..5b7a9728d2 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -321,6 +321,11 @@ sc_prohibit_internal_functions:
>   halt='use VIR_ macros instead of internal functions' \
> $(_sc_search_regexp)
>
> +sc_prohibit_raw_virclassnew:
> + @prohibit='virClassNew *\(' \
> + halt='use VIR_CLASS_NEW macro instead of virClassNew' \

s/macro//

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/9] virobject: Introduce VIR_CLASS_NEW() macro

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:14PM +0200, Michal Privoznik wrote:
> So far we are repeating the following lines over and over:
>
>   virClassNew(virClassForObject(),
>   "virSomeObject",
>   sizeof(virSomeObject),
>   virSomeObjectDispose);
>
> While this works, it is impossible to do some checking. Firstly,
> the class name (the 2nd argument) doesn't match the name in the
> code in all cases (the 3rd argument). Secondly, the current style
> is needlessly verbose. This commit turns example into following:
>
>   VIR_CLASS_NEW(virClassForObject(),
> virSomeObject);
>
> Signed-off-by: Michal Privoznik 

...

> diff --git a/src/interface/interface_backend_netcf.c 
> b/src/interface/interface_backend_netcf.c
> index cc2402febb..8acccf8940 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -55,10 +55,8 @@ static void virNetcfDriverStateDispose(void *obj);
>  static int
>  virNetcfDriverStateOnceInit(void)
>  {
> -if (!(virNetcfDriverStateClass = virClassNew(virClassForObjectLockable(),
> -   "virNetcfDriverState",
> -   sizeof(virNetcfDriverState),
> -   virNetcfDriverStateDispose)))
> +if (!(virNetcfDriverStateClass = 
> VIR_CLASS_NEW(virClassForObjectLockable(),
> +   virNetcfDriverStat)))

There's a typo in the 2nd parameter which makes the patch fail the compilation.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/9] Introduce virNetSASLContextDispose

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:13PM +0200, Michal Privoznik wrote:
> Strictly speaking this is not needed right now. However, next

The first sentence only makes sense in the context of the patch series review,
that will not be the case for someone looking at it in the future - plain
"Future commits rely on the presence of this callback" should suffice.

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/9] virobject: Introduce VIR_CLASS_NEW() macro

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:14PM +0200, Michal Privoznik wrote:
> So far we are repeating the following lines over and over:
>
>   virClassNew(virClassForObject(),
>   "virSomeObject",
>   sizeof(virSomeObject),
>   virSomeObjectDispose);
>
> While this works, it is impossible to do some checking. Firstly,
> the class name (the 2nd argument) doesn't match the name in the
> code in all cases (the 3rd argument). Secondly, the current style
> is needlessly verbose. This commit turns example into following:
>
>   VIR_CLASS_NEW(virClassForObject(),
> virSomeObject);
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/access/viraccessmanager.c   |   6 +-
>  src/bhyve/bhyve_conf.c  |   6 +-
>  src/conf/capabilities.c |   6 +-
>  src/conf/domain_capabilities.c  |  12 +--
>  src/conf/domain_conf.c  |  18 ++---
>  src/conf/domain_event.c | 126 
> +++-
>  src/conf/network_event.c|  12 +--
>  src/conf/node_device_event.c|  18 ++---
>  src/conf/object_event.c |  12 +--
>  src/conf/secret_event.c |  18 ++---
>  src/conf/storage_event.c|  18 ++---
>  src/conf/virdomainobjlist.c |   6 +-
>  src/conf/virinterfaceobj.c  |  12 +--
>  src/conf/virnetworkobj.c|  12 +--
>  src/conf/virnodedeviceobj.c |  12 +--
>  src/conf/virsecretobj.c |  12 +--
>  src/conf/virstorageobj.c|  24 ++
>  src/datatypes.c |   6 +-
>  src/interface/interface_backend_netcf.c |   6 +-
>  src/libvirt-admin.c |   6 +-
>  src/libxl/libxl_conf.c  |   6 +-
>  src/libxl/libxl_domain.c|   6 +-
>  src/libxl/libxl_migration.c |   6 +-
>  src/logging/log_handler.c   |   6 +-
>  src/lxc/lxc_conf.c  |   6 +-
>  src/lxc/lxc_monitor.c   |   6 +-
>  src/node_device/node_device_udev.c  |   6 +-
>  src/qemu/qemu_agent.c   |   6 +-
>  src/qemu/qemu_capabilities.c|   6 +-
>  src/qemu/qemu_conf.c|   6 +-
>  src/qemu/qemu_domain.c  |  36 +++--
>  src/qemu/qemu_monitor.c |   6 +-
>  src/rpc/virkeepalive.c  |   6 +-
>  src/rpc/virnetclient.c  |   6 +-
>  src/rpc/virnetclientprogram.c   |   6 +-
>  src/rpc/virnetclientstream.c|   6 +-
>  src/rpc/virnetdaemon.c  |   6 +-
>  src/rpc/virnetlibsshsession.c   |   6 +-
>  src/rpc/virnetsaslcontext.c |  12 +--
>  src/rpc/virnetserver.c  |   6 +-
>  src/rpc/virnetserverclient.c|   6 +-
>  src/rpc/virnetserverprogram.c   |   6 +-
>  src/rpc/virnetserverservice.c   |   6 +-
>  src/rpc/virnetsocket.c  |   6 +-
>  src/rpc/virnetsshsession.c  |   6 +-
>  src/rpc/virnettlscontext.c  |  12 +--
>  src/security/security_manager.c |   6 +-
>  src/util/virclosecallbacks.c|   6 +-
>  src/util/virdnsmasq.c   |   7 +-
>  src/util/virfdstream.c  |   6 +-
>  src/util/virfilecache.c |   6 +-
>  src/util/virhash.c  |   6 +-
>  src/util/virhostdev.c   |   6 +-
>  src/util/viridentity.c  |   6 +-
>  src/util/virmacmap.c|   6 +-
>  src/util/virmdev.c  |   6 +-
>  src/util/virobject.c|  12 +--
>  src/util/virobject.h|   4 +
>  src/util/virpci.c   |   6 +-
>  src/util/virportallocator.c |   6 +-
>  src/util/virresctrl.c   |  12 +--
>  src/util/virscsi.c  |   6 +-
>  src/util/virscsivhost.c |   6 +-
>  src/util/virusb.c   |   6 +-
>  src/vbox/vbox_common.c  |   6 +-
>  src/vz/vz_driver.c  |   6 +-
>  tests/virfilecachetest.c|   6 +-

...

> diff --git a/src/datatypes.c b/src/datatypes.c
> index 0c3c66a9ce..3016e45076 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -74,10 +74,8 @@ static int
>  virDataTypesOnceInit(void)
>  {
>  #define DECLARE_CLASS_COMMON(basename, parent) \
> -if (!(basename ## Class = virClassNew(parent, \
> -  #basename, \
> -  sizeof(basename), \
> -  basename ## Dispose))) \
> +if (!(basename ## Class = VIR_CLASS_NEW(parent, \
> +basename))) \
>  return -1;

I'd probably go one step further and move the DECLARE_CLASS macros in a widely
accessible header and use that 

Re: [libvirt] [PATCH 2/2] lxc: Clean up /.oldroot

[Daniel P . Berrangé]

On Sun, Apr 15, 2018 at 04:30:11PM +0100, Radostin Stoyanov wrote:
> Remove the /.oldroot directory after it has been unmounted (at the end
> of lxcContainerSetupPivotRoot). Ignore errors silently.
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  src/lxc/lxc_container.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 665b93a0a..dd4e38703 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -1811,6 +1811,9 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr 
> vmDef,
>  if (lxcContainerUnmountSubtree("/.oldroot", true) < 0)
>  goto cleanup;
>  
> +if (virFileRemove("/.oldroot", 0, 0) < 0)
> +VIR_DEBUG("Failed to remove /.oldroot after start");
> +

I think this introduces a race condition. There can be two containers
with the same root filesystem. If we start both at the same time, then
this deletion of /.oldroot can cause the other contanier to fail to
start if it saw that /.oldroot already existed & it thus tried to skip
mkdir.

Leaving the empty directory is harmless IMHO

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Only attempt to fix intltool-update if it exists

[Pavel Hrdina]

On Mon, Apr 16, 2018 at 10:56:02AM +0200, Andrea Bolognani wrote:
> On Mon, 2018-04-16 at 10:43 +0200, Andrea Bolognani wrote:
> > On Mon, 2018-04-16 at 10:29 +0200, Pavel Hrdina wrote:
> > > We need to probably split kludges task into two separate tasks where
> > > one will be executed before installing project dependencies to fix
> > > base-os issues and second one executed after all dependencies are
> > > installed to fix the remaining issues.
> > 
> > Mh, I kinda wanted to avoid doing that, but I guess it's way more
> > user-friendly to do it the way you suggested. v2 coming right up :)
> 
> Actually, we can keep it simple by including tasks/kludges.yml a
> second time *after* installing packages. All changes performed in
> there are idempotent, so it will add pretty much no execution time
> to a 'lcitool update' run and avoids having to split the task.
> 
> Basically I would squash in:
> 
> - 8< - 8< -
> diff --git a/guests/site.yml b/guests/site.yml
> index 26127be..d208e5d 100644
> --- a/guests/site.yml
> +++ b/guests/site.yml
> @@ -55,3 +55,7 @@
>  - include: tasks/jenkins.yml
>when:
>  - flavor == 'jenkins'
> +
> +# Some of the kludges involve tweaking files that are included in
> +# the packages we just installed, so go through them again here
> +- include: tasks/kludges.yml
> - >8 - >8 -
> 
> Does that sound reasonable?

Right, that's good enough.

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Only attempt to fix intltool-update if it exists

[Andrea Bolognani]

On Mon, 2018-04-16 at 10:43 +0200, Andrea Bolognani wrote:
> On Mon, 2018-04-16 at 10:29 +0200, Pavel Hrdina wrote:
> > We need to probably split kludges task into two separate tasks where
> > one will be executed before installing project dependencies to fix
> > base-os issues and second one executed after all dependencies are
> > installed to fix the remaining issues.
> 
> Mh, I kinda wanted to avoid doing that, but I guess it's way more
> user-friendly to do it the way you suggested. v2 coming right up :)

Actually, we can keep it simple by including tasks/kludges.yml a
second time *after* installing packages. All changes performed in
there are idempotent, so it will add pretty much no execution time
to a 'lcitool update' run and avoids having to split the task.

Basically I would squash in:

- 8< - 8< -
diff --git a/guests/site.yml b/guests/site.yml
index 26127be..d208e5d 100644
--- a/guests/site.yml
+++ b/guests/site.yml
@@ -55,3 +55,7 @@
 - include: tasks/jenkins.yml
   when:
 - flavor == 'jenkins'
+
+# Some of the kludges involve tweaking files that are included in
+# the packages we just installed, so go through them again here
+- include: tasks/kludges.yml
- >8 - >8 -

Does that sound reasonable?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Add icoutils dependency to virt-viewer project

[Andrea Bolognani]

On Mon, 2018-04-16 at 10:31 +0200, Pavel Hrdina wrote:
> On Fri, Apr 13, 2018 at 02:10:08PM +0200, Andrea Bolognani wrote:
> > The icotool command is used to manipulate Windows icon files.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  guests/vars/mappings.yml | 4 
> >  guests/vars/projects/virt-viewer.yml | 1 +
> >  2 files changed, 5 insertions(+)
> 
> Isn't this required only for mingw build?

Yeah, you're right.

> If so, we might want to
> introduce virt-viewer+mingw project.

That was the plan all along :)

I'll wait for Dan to push his patches and then post a series that
splits off the +mingw part and also adds the icotool dependency.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Only attempt to fix intltool-update if it exists

[Andrea Bolognani]

On Mon, 2018-04-16 at 10:29 +0200, Pavel Hrdina wrote:
> How this will work to fix the FreeBSD installation for the
> freshly-provisioned guest? This would require running
> './lcitool prepare $guest' and again './lcitool update $guest'.

Correct.

> We need to probably split kludges task into two separate tasks where
> one will be executed before installing project dependencies to fix
> base-os issues and second one executed after all dependencies are
> installed to fix the remaining issues.

Mh, I kinda wanted to avoid doing that, but I guess it's way more
user-friendly to do it the way you suggested. v2 coming right up :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/9] src: Unify dispose function names

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:12PM +0200, Michal Privoznik wrote:
> If a function is disposing virSomething it should be called
> virSomethingDispose(). There are two offenders:
> virCapabilitiesDispose(virCapsPtr) and
> virDomainXMLOptionClassDispose(virDomainXMLOptionPtr).
>
> Signed-off-by: Michal Privoznik 
> ---

An idea for a related patch: there are a few violators that put
"virSomethingClass" is the name of the class, while the general trend is to
omit "Class" from the name (it also doesn't make that much sense to have it
there).

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Add icoutils dependency to virt-viewer project

[Pavel Hrdina]

On Fri, Apr 13, 2018 at 02:10:08PM +0200, Andrea Bolognani wrote:
> The icotool command is used to manipulate Windows icon files.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/vars/mappings.yml | 4 
>  guests/vars/projects/virt-viewer.yml | 1 +
>  2 files changed, 5 insertions(+)

Isn't this required only for mingw build?  If so, we might want to
introduce virt-viewer+mingw project.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Only attempt to fix intltool-update if it exists

[Pavel Hrdina]

On Fri, Apr 13, 2018 at 06:52:20PM +0200, Andrea Bolognani wrote:
> If we're running against a freshly-provisioned FreeBSD guest,
> intltool won't have been installed yet and attempts to fix it
> will fail. Make sure the file exists.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/tasks/kludges.yml | 8 
>  1 file changed, 8 insertions(+)

How this will work to fix the FreeBSD installation for the
freshly-provisioned guest? This would require running 
'./lcitool prepare $guest' and again './lcitool update $guest'.

We need to probably split kludges task into two separate tasks where
one will be executed before installing project dependencies to fix
base-os issues and second one executed after all dependencies are
installed to fix the remaining issues.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Set remote_user at the playbook level

[Pavel Hrdina]

On Wed, Apr 11, 2018 at 02:36:15PM +0200, Andrea Bolognani wrote:
> This is very convenient for developers using the 'test' flavor,
> because it ensures
> 
>   $ ./lcitool update all
> 
> still works by logging in as root, but at the same time ad-hoc
> invocations such as
> 
>   $ ansible all -m shell -a 'cd libvirt && $MAKE -j check'
> 
> and custom playbooks log in as the 'test' user instead, and as
> such have all the usual environment variables ($MAKE, $PYTHON,
> $PATH, $VIRT_PREFIX and friends) available to them, which makes
> testing changes locally a breeze.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/group_vars/all/main.yml | 2 --
>  guests/site.yml| 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH v2] guests: Clean up packages after update

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 12:13:23PM +0200, Andrea Bolognani wrote:
> The package cache can grow to eat up a lot of disk space, and
> not removing unused packages can lead to upgrade issues down
> the line for fast moving distributions such as Fedora Rawhide.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> Changes from [v1]:
> 
>   * call 'yum autoremove' when possible.
> 
> [v1] https://www.redhat.com/archives/libvir-list/2018-April/msg00970.html
> 
>  guests/tasks/base.yml | 28 
>  1 file changed, 28 insertions(+)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/9] domain_event: s/MetadataCange/MetadataChange/g

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:08PM +0200, Michal Privoznik wrote:
> There's a typo in struct name.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_event.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/9] src: Unify virObject member name

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:11PM +0200, Michal Privoznik wrote:
> Whenever we declare a new object the first member of the struct
> has to be virObject (or any other member of that family). Now, up
> until now we did not care about the name of the struct member.
> But lets unify it so that we can do some checks at compile time
> later.
>
> The unified name is 'parent'.
>
> Signed-off-by: Michal Privoznik 

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] guests: Introduce libvirt+mingw project

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 06:06:30PM +0200, Andrea Bolognani wrote:
> Instead of cramming the dependencies that we need for a MinGW
> build of libvirt along with those we need for a native build,
> create a new project for the former.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/host_vars/libvirt-fedora-rawhide/main.yml |  1 +
>  guests/vars/projects/libvirt+mingw.yml   | 28 
> 
>  guests/vars/projects/libvirt.yml | 24 
>  3 files changed, 29 insertions(+), 24 deletions(-)
>  create mode 100644 guests/vars/projects/libvirt+mingw.yml

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu_monitor_json: Build Json CPU Model Info

[Erik Skultety]

On Mon, Apr 16, 2018 at 01:06:57AM -0500, Chris Venteicher wrote:
> Function qemuMonitorJSONBuildCPUModelInfoJSON builds Json of form
> {"model": {"name": "IvyBridge", "props": {}}}
> from pointer to qemuMonitorCPUModelInfo.
> ---
>  src/qemu/qemu_monitor_json.c | 49 
> 
>  1 file changed, 49 insertions(+)

...

> +
> +switch (prop->type) {
> +case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN:
> +if (virJSONValueObjectAppendBoolean(cpu_props, prop->name, 
> prop->value.boolean) < 0)
> +goto cleanup;
> +break;
> +
> +case QEMU_MONITOR_CPU_PROPERTY_STRING:
> +if (virJSONValueObjectAppendString(cpu_props, prop->name, 
> prop->value.string) < 0)
> +goto cleanup;
> +break;
> +
> +case QEMU_MONITOR_CPU_PROPERTY_NUMBER:
> +if (virJSONValueObjectAppendNumberLong(cpu_props, prop->name, 
> prop->value.number) < 0)
> +goto cleanup;
> +break;
> +
> +case QEMU_MONITOR_CPU_PROPERTY_LAST:
> +break;

The most recent "correct" way to write switches in libvirt is to do the
following:
case QEMU_MONITOR_CPU_PROPERTY_LAST:
default:
virReportEnumRangeError(qemuMonitorCPUPropertyPtr, prop->type);
goto cleanup;

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] qemu_monitor_json: Populate CPUModelInfo struct from json

[Erik Skultety]

On Mon, Apr 16, 2018 at 01:06:56AM -0500, Chris Venteicher wrote:
> New function qemuMonitorJSONBuildCPUModelInfo created by extracting code
> from existing function qemuMonitorJSONGetCPUModelExpansion to create a
> reusable function for extracting cpu model info from json.
> ---
>  src/qemu/qemu_monitor_json.c | 82 
> ++--
>  1 file changed, 56 insertions(+), 26 deletions(-)
>
...

> +if (virJSONValueObjectForeachKeyValue(cpu_props,
> +  
> qemuMonitorJSONParseCPUModelProperty,
> +  machine_model) < 0)
> +goto cleanup;
> +
> +ret = 0;
> +*model = machine_model;
> +machine_model = NULL;

we also tend to use VIR_STEAL_PTR(dst, src) in ^this case.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu_monitor: query-cpu-model-baseline QMP command

[Ján Tomko]

On Mon, Apr 16, 2018 at 01:06:58AM -0500, Chris Venteicher wrote:

Function qemuMonitorGetCPUModelBaseline exposed to carry out a QMP
query-cpu-model-baseline transaction with QEMU.

QEMU determines a baseline CPU Model from two input CPU Models to
complete the query-cpu-model-baseline transaction.
---
src/qemu/qemu_monitor.c  | 16 +
src/qemu/qemu_monitor.h  |  5 
src/qemu/qemu_monitor_json.c | 56 
src/qemu/qemu_monitor_json.h |  7 ++
4 files changed, 84 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 7b647525b..9db9d4b81 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3874,6 +3874,22 @@ qemuMonitorCPUModelInfoCopy(const 
qemuMonitorCPUModelInfo *orig)
return NULL;
}

+int
+qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
+   qemuMonitorCPUModelInfoPtr model_a,
+   qemuMonitorCPUModelInfoPtr model_b,
+   qemuMonitorCPUModelInfoPtr *model_baseline)
+{
+if (model_a)
+VIR_DEBUG("model_a->name=%s", model_a->name);
+
+if (model_b)
+VIR_DEBUG("model_b->name=%s", model_b->name);
+
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONGetCPUModelBaseline(mon, model_a, model_b, 
model_baseline);
+}

int
qemuMonitorGetCommands(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index d04148e56..c7a80ca63 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1079,6 +1079,11 @@ void 
qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
qemuMonitorCPUModelInfoPtr
qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);

+int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
+   qemuMonitorCPUModelInfoPtr model_a,
+   qemuMonitorCPUModelInfoPtr model_b,
+   qemuMonitorCPUModelInfoPtr *model_baseline);
+
int qemuMonitorGetCommands(qemuMonitorPtr mon,
   char ***commands);
int qemuMonitorGetEvents(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 320d4601e..e03f6091c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5544,6 +5544,62 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
return ret;
}

+int
+qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
+   qemuMonitorCPUModelInfoPtr model_a,
+   qemuMonitorCPUModelInfoPtr model_b,
+   qemuMonitorCPUModelInfoPtr *model_baseline)
+{
+int ret = -1;
+virJSONValuePtr cmd   = NULL;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data  = NULL;
+virJSONValuePtr modela = NULL;
+virJSONValuePtr modelb = NULL;


Please do not try to align the =.


+
+*model_baseline = NULL;
+
+if (qemuMonitorJSONBuildCPUModelInfoJSON(model_a, ) < 0)
+goto cleanup;
+
+if (qemuMonitorJSONBuildCPUModelInfoJSON(model_b, ) < 0)
+goto cleanup;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline",
+   "a:modela", ,
+   "a:modelb", ,
+   NULL)))
+goto cleanup;
+
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+goto cleanup;
+
+/* Urgh, some QEMU architectures have query-cpu-model-baseline
+ * command but return 'GenericError' with string "Not supported",
+ * instead of simply omitting the command entirely
+ */
+if (qemuMonitorJSONHasError(reply, "GenericError"))
+goto cleanup;


Missing virReportError.

Ideally, on error all libvirt functions would either call virReportError
or be quiet in all possible exit paths.

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/9] util: Make structs follow our naming convention

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:09PM +0200, Michal Privoznik wrote:
> There are two structs virMacMap and virFDStreamData that don't
> have the underscore prefix. Put it there so that they follow the
> rest of the code.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virfdstream.c | 4 ++--
>  src/util/virmacmap.c   | 2 +-
>  src/util/virmacmap.h   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

these suffer from the same problem:
daemonAdmClientPrivate
virDomainQemuMonitorEventData
xentoollog_logger_libvirt (no convention here at all, so I'd skip this one)
virLXCMeminfo
qemuBlockNodeNameBackingChainData
daemonClientStream
virNetMessageHeader
virNetMessageError
virCgroup
virNetlinkCallbackData
virPerf
virPerfEvent
virPerfEventAttr
virRotatingFileWriterEntry
virRotatingFileReaderEntry
virRotatingFileWriter
virRotatingFileReader
virMutex
virRWLock
virCond
virThreadLocal
virThread
virTypedParameterRemoteValue (the second typedef is completely wrong):
typedef struct _virTypedParameterRemoteValue virTypedParameterRemoteValue;
typedef struct virTypedParameterRemoteValue 
*virTypedParameterRemoteValuePtr;
virOnceControl
vbox - basically all of the structs :P
vzDomObj

Honestly, given the number of places where this should be fixed, I'm not sure
whether we should really go with the patch, but at the same time, I can imagine
us having this unified once and for all.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] qemu_monitor_json: Populate CPUModelInfo struct from json

[Ján Tomko]

On Mon, Apr 16, 2018 at 01:06:56AM -0500, Chris Venteicher wrote:

New function qemuMonitorJSONBuildCPUModelInfo created by extracting code
from existing function qemuMonitorJSONGetCPUModelExpansion to create a
reusable function for extracting cpu model info from json.
---
src/qemu/qemu_monitor_json.c | 82 ++--
1 file changed, 56 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 57c2c4de0..cf31c16a0 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5337,6 +5337,61 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
return 0;
}

+// model_json: {"model": {"name": "IvyBridge", "props": {}}}


/* comment */

and

/*
* comment
*/

are the preferred comment styles in libvirt.
https://libvirt.org/hacking.html#formatting


+static int


This function either returns -1 when model is NULL or 0 when it's not.
You can return qemuMonitorCPUModelInfoPtr directly.


+qemuMonitorJSONBuildCPUModelInfo(virJSONValuePtr model_json,


Put 'FromJSON' at the end of the function name, e.g.:
qemuMonitorJSONGetCPUModelInfoFromJSON


+ qemuMonitorCPUModelInfoPtr *model)
+{
+virJSONValuePtr cpu_model;
+virJSONValuePtr cpu_props;
+qemuMonitorCPUModelInfoPtr machine_model = NULL;
+int ret = -1;
+char const *cpu_name;
+
+*model = NULL;
+


Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH] tests: Avoid hidding TimeoutError in main loop

[Ján Tomko]

On Mon, Apr 16, 2018 at 09:20:30AM +0200, Katerina Koukiou wrote:

Currently the error was not propagated from the timeout
handler properly, and we had PASSED tests even with
timeouts.

Signed-off-by: Katerina Koukiou 
---
test/libvirttest.py | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:10PM +0200, Michal Privoznik wrote:
> In next patches this name will be needed for a different memeber.
> Also, it makes sense to rename the variable because it does not
> contain reference to parent device, just its name.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/virnodedeviceobj.c  | 2 +-
>  src/datatypes.c  | 2 +-
>  src/datatypes.h  | 2 +-
>  src/libvirt-nodedev.c| 6 +++---
>  src/node_device/node_device_driver.c | 4 ++--
>  src/test/test_driver.c   | 6 +++---
>  6 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index ad0f27ee47..9d2996046f 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -870,7 +870,7 @@ virNodeDeviceObjListExportCallback(void *payload,
>  virNodeDeviceMatch(obj, data->flags)) {
>  if (data->devices) {
>  if (!(device = virGetNodeDevice(data->conn, def->name)) ||
> -VIR_STRDUP(device->parent, def->parent) < 0) {
> +VIR_STRDUP(device->parentName, def->parent) < 0) {
>  virObjectUnref(device);
>  data->error = true;
>  goto cleanup;
> diff --git a/src/datatypes.c b/src/datatypes.c
> index f7eef24ba8..0c3c66a9ce 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -653,7 +653,7 @@ virNodeDeviceDispose(void *obj)
>  VIR_DEBUG("release dev %p %s", dev, dev->name);
>
>  VIR_FREE(dev->name);
> -VIR_FREE(dev->parent);
> +VIR_FREE(dev->parentName);
>
>  virObjectUnref(dev->conn);
>  }
> diff --git a/src/datatypes.h b/src/datatypes.h
> index 1a8ea01ba3..66733b075c 100644
> --- a/src/datatypes.h
> +++ b/src/datatypes.h
> @@ -618,7 +618,7 @@ struct _virNodeDevice {
>  virObject object;
>  virConnectPtr conn; /* pointer back to the connection */
>  char *name; /* device name (unique on node) */
> -char *parent;   /* parent device name */
> +char *parentName;   /* parent device name */
>  };
>
>  /**
> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
> index 563ce889b9..8ced3cea0e 100644
> --- a/src/libvirt-nodedev.c
> +++ b/src/libvirt-nodedev.c
> @@ -346,16 +346,16 @@ virNodeDeviceGetParent(virNodeDevicePtr dev)
>
>  virCheckNodeDeviceReturn(dev, NULL);
>
> -if (!dev->parent) {
> +if (!dev->parentName) {
>  if (dev->conn->nodeDeviceDriver && 
> dev->conn->nodeDeviceDriver->nodeDeviceGetParent) {
> -dev->parent = 
> dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);
> +dev->parentName = 
> dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);

Since you're adjusting the struct member name, you could go as far as fixing
the *GetParent accessor's name too.

With that:
Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv3 3/6] tests: add qemumonitorjson tests for query-cpus-fast

[Viktor VM Mihajlovski]

On 12.04.2018 17:52, John Ferlan wrote:
> 
> 
> On 04/04/2018 10:45 AM, Viktor Mihajlovski wrote:
>> Extended the json monitor test program with support for query-cpus-fast
>> and added a sample file set for x86 data obtained using the it.
>> Also extend the test program to recognize the halted property.
> 
> This last sentence involves code that probably should be separated into
> its own patch since it's unrelated to the new data. If it's moved this
> patch into one previously, then this patch/adjustment "proves" nothing
> changes (which is good).
> 
> I can do that for you so you don't have to post another series.  I would
> make you the author and add your S-o-b... Of course I need you to say OK
> for that too!
> 
> The rest of this shows what I'd extract...
> Good point. I will gladly accept your offer to split out the 'halted' part.
[...]

-- 
Regards,
  Viktor Mihajlovski

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list