Re: [PATCH 04/25] util: validate return from xmlNodeGetContent before use
On 6/25/20 6:55 PM, Ján Tomko wrote: On a Wednesday in 2020, Laine Stump wrote: There were a few uses of xmlNodeGetContent() that didn't check for NULL before using the result. A NULL return from xmlNodeGetContent() *could* (probably does) mean that there was an Out of Memory condition, but it is unclear from the documentation if that is always the case, or if it could just indicate a missing value in the document, so we don't report an OOM error, but just don't try to use it for, e.g., conversion to an integer. Is it possible to have an element with "no value"? I never found anywhere that said "No". But I also never found anywhere that says "yes", so I opted for "do no harm" (or something like that). Even gives me an empty string instead of NULL. Okay, *that* says "No". So I'll change the patch to always report an OOM error. Jano Signed-off-by: Laine Stump --- src/conf/domain_conf.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-)
Re: [PATCH 03/25] conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference
On 6/26/20 6:54 PM, Laine Stump wrote: On 6/25/20 6:44 PM, Ján Tomko wrote: and give a possibility of assigning the path directly to 'path', without the extra steal_pointer. I don't follow there - if you assign directly from xmlNodeGetContent() into path, then you'll need to duplicate the virReportOOMError(). Sigh. Nevermind. It had already been several days since I wrote the code, so I'd completely forgotten it and hadn't looked back at the bottom yet (where I would have seen the "extra steal pointer" you mention). Now I get it.
Re: [PATCH 03/25] conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference
On 6/25/20 6:44 PM, Ján Tomko wrote:
On a Wednesday in 2020, Laine Stump wrote:
virDomainBlkioDeviceParseXML() has multiple cases of sending the
return from xmlNodeGetContent() directly to virStrToLong_xx() without
checking for NULL. Although it is *very* rare for xmlNodeGetContent()
to return NULL (possibly it only happens in an OOM condition? The
documentation is unclear), it could happen, and the refactor in this
patch manages to eliminate several lines of repeated code while adding
in a (single) check for NULL.
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 39 +++
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1916b51d38..8cde1cd0e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
virBlkioDevicePtr dev)
{
xmlNodePtr node;
- g_autofree char *c = NULL;
+ g_autofree char *path = NULL;
node = root->children;
while (node) {
- if (node->type == XML_ELEMENT_NODE) {
- if (virXMLNodeNameEqual(node, "path") && !dev->path) {
- dev->path = (char *)xmlNodeGetContent(node);
+ g_autofree char *c = NULL;
+
+ if (node->type == XML_ELEMENT_NODE &&
+ (c = (char *)xmlNodeGetContent(node))) {
Missing ErrorReport if xmlNodeGetContent fails.
Well, I was uncertain whether or not it was *always* an error. The API
docs don't specifically say, a google search revealed people asking the
question, but nobody answering it definitively (I think there may have
been some snarky condescending reply on stackexchange (par for the
course), but no actual information), and I stopped trying to figure it
out by looking at the libxml2 source after just a couple layers - ain't
nobody got time for that!
But you apparently tried it out and determined that it will return ""
rather than NULL as long as node->type == XML_ELEMENT_NODE, so I'll
trust that and treat all NULL returns as OOM (including in a later patch).
Converting this open-coded for loop to an actual for loop would
grant us 'continue' privileges, which would make the checks
nicer
If you're averse to "else if" I guess.
and give a possibility of assigning the path directly
to 'path', without the extra steal_pointer.
I don't follow there - if you assign directly from xmlNodeGetContent()
into path, then you'll need to duplicate the virReportOOMError().
Anyway, I'll turn it into a for() loop make the NULL return from
xmlNodeGetContent() an error (rather than ignoring it) and resubmit,
since it's too many changes to trust me on it.
Alternatively, the minimum diff where I'd consider this patch to be
a strict improvement is:
} else if (node->type == XML_ELEMENT_NODE && !c) {
virReportOOMError();
return -1;
}
With that: Reviewed-by: Ján Tomko
Jano
[PATCH 3/7] virsh-domain.c: modernize virshVcpuinfoInactive()
Use g_auto* in the string and in the bitmap. Remove the
cleanup label since it's now unneeded.
Signed-off-by: Daniel Henrique Barboza
---
tools/virsh-domain.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 085b88b097..23d41a4ddf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6886,16 +6886,15 @@ virshVcpuinfoInactive(vshControl *ctl,
int maxcpu,
bool pretty)
{
-unsigned char *cpumaps = NULL;
+g_autofree unsigned char *cpumaps = NULL;
size_t cpumaplen;
int ncpus;
-virBitmapPtr vcpus = NULL;
+g_autoptr(virBitmap) vcpus = NULL;
ssize_t nextvcpu = -1;
-bool ret = false;
bool first = true;
if (!(vcpus = virshDomainGetVcpuBitmap(ctl, dom, true)))
-goto cleanup;
+return false;
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
cpumaps = vshMalloc(ctl, virBitmapSize(vcpus) * cpumaplen);
@@ -6903,7 +6902,7 @@ virshVcpuinfoInactive(vshControl *ctl,
if ((ncpus = virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus),
cpumaps, cpumaplen,
VIR_DOMAIN_AFFECT_CONFIG)) < 0)
-goto cleanup;
+return false;
while ((nextvcpu = virBitmapNextSetBit(vcpus, nextvcpu)) >= 0) {
if (!first)
@@ -6918,15 +6917,10 @@ virshVcpuinfoInactive(vshControl *ctl,
if (virshVcpuinfoPrintAffinity(ctl,
VIR_GET_CPUMAP(cpumaps, cpumaplen,
nextvcpu),
maxcpu, pretty) < 0)
-goto cleanup;
+return false;
}
-ret = true;
-
- cleanup:
-virBitmapFree(vcpus);
-VIR_FREE(cpumaps);
-return ret;
+return true;
}
--
2.26.2
[PATCH 5/7] virhostcpu.c: refactor virHostCPUParseCountLinux()
This function reads the string in sysfspath/cpu/present and
parses it manually to retrieve the number of present CPUs.
virHostCPUGetPresentBitmap() reads and parses the same file,
using a more robust parser via virBitmapParseUnlimited(),
but returns a bitmap. Let's drop all the manual parsing done
here and simply return the size of the resulting bitmap
from virHostCPUGetPresentBitmap().
Given that no more parsing is being done manually in the function,
rename it to virHostCPUCountLinux().
Signed-off-by: Daniel Henrique Barboza
---
src/util/virhostcpu.c | 30 +++---
1 file changed, 7 insertions(+), 23 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 3023bca831..615250d05d 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -856,33 +856,17 @@ virHostCPUGetStatsLinux(FILE *procstat,
}
-/* Determine the number of CPUs (maximum CPU id + 1) from a file containing
- * a list of CPU ids, like the Linux sysfs cpu/present file */
+/* Determine the number of CPUs (maximum CPU id + 1) present in
+ * the host. */
static int
-virHostCPUParseCountLinux(void)
+virHostCPUCountLinux(void)
{
-char *str = NULL;
-char *tmp;
-int ret = -1;
+g_autoptr(virBitmap) present = virHostCPUGetPresentBitmap();
-if (virFileReadValueString(, "%s/cpu/present", SYSFS_SYSTEM_PATH) < 0)
+if (!present)
return -1;
-tmp = str;
-do {
-if (virStrToLong_i(tmp, , 10, ) < 0 ||
-!strchr(",-", *tmp)) {
-virReportError(VIR_ERR_NO_SUPPORT,
- _("failed to parse %s"), str);
-ret = -1;
-goto cleanup;
-}
-} while (*tmp++ && *tmp);
-ret++;
-
- cleanup:
-VIR_FREE(str);
-return ret;
+return virBitmapSize(present);
}
#endif
@@ -1031,7 +1015,7 @@ int
virHostCPUGetCount(void)
{
#if defined(__linux__)
-return virHostCPUParseCountLinux();
+return virHostCPUCountLinux();
#elif defined(__FreeBSD__) || defined(__APPLE__)
return virHostCPUGetCountAppleFreeBSD();
#else
--
2.26.2
[PATCH 6/7] virhostcpu.c: introduce virHostCPUGetAvailableCPUsBitmap()
The idea is to have a function that calls virHostCPUGetOnlineBitmap()
but, instead of returning NULL if the host does not have CPU
offlining capabilities, fall back to a bitmap containing all
present CPUs.
Next patch will use this helper in two other places.
Signed-off-by: Daniel Henrique Barboza
---
src/libvirt_private.syms | 1 +
src/util/virhostcpu.c| 30 ++
src/util/virhostcpu.h| 2 ++
3 files changed, 33 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ae0e253ab9..f120e200cb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2211,6 +2211,7 @@ virHookPresent;
# util/virhostcpu.h
+virHostCPUGetAvailableCPUsBitmap;
virHostCPUGetCount;
virHostCPUGetInfo;
virHostCPUGetKVMMaxVCPUs;
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 615250d05d..8ca67e357d 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1099,6 +1099,36 @@ virHostCPUGetMap(unsigned char **cpumap,
}
+/* virHostCPUGetAvailableCPUsBitmap():
+ *
+ * Returns a virBitmap object with all available host CPUs.
+ *
+ * This is a glorified wrapper of virHostCPUGetOnlineBitmap()
+ * that, instead of returning NULL when 'ifndef __linux__' and
+ * the caller having to handle it outside the function, returns
+ * a virBitmap with all the possible CPUs in the host, up to
+ * virHostCPUGetCount(). */
+virBitmapPtr
+virHostCPUGetAvailableCPUsBitmap(void)
+{
+g_autoptr(virBitmap) bitmap = NULL;
+
+if (!(bitmap = virHostCPUGetOnlineBitmap())) {
+int hostcpus;
+
+if ((hostcpus = virHostCPUGetCount()) < 0)
+return NULL;
+
+if (!(bitmap = virBitmapNew(hostcpus)))
+return NULL;
+
+virBitmapSetAll(bitmap);
+}
+
+return g_steal_pointer();
+}
+
+
#if HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT)
/* Get the number of threads per subcore.
diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index 48b1431ca4..d07503857e 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -43,6 +43,8 @@ int virHostCPUGetStats(int cpuNum,
bool virHostCPUHasBitmap(void);
virBitmapPtr virHostCPUGetPresentBitmap(void);
virBitmapPtr virHostCPUGetOnlineBitmap(void);
+virBitmapPtr virHostCPUGetAvailableCPUsBitmap(void);
+
int virHostCPUGetCount(void);
int virHostCPUGetThreadsPerSubcore(virArch arch) G_GNUC_NO_INLINE;
--
2.26.2
[PATCH 1/7] qemu_driver.c: use g_autoptr in qemuDomainGetEmulatorPinInfo()
Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5b38b3d24..d486ce5278 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5427,7 +5427,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, int ret = -1; int hostcpus; virBitmapPtr cpumask = NULL; -virBitmapPtr bitmap = NULL; +g_autoptr(virBitmap) bitmap = NULL; virBitmapPtr autoCpuset = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -5468,7 +5468,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, cleanup: virDomainObjEndAPI(); -virBitmapFree(bitmap); return ret; } -- 2.26.2
[PATCH 0/7] consider available CPUs in vcpupin/emulatorpin output
Hi, This series contains 5 cleanups and refactorings I found on my way to fixing [1]. Last 2 patches contains the actual fix for the bug. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1434276 Daniel Henrique Barboza (7): qemu_driver.c: use g_autoptr in qemuDomainGetEmulatorPinInfo() virhostcpu.c: use g_autoptr in virHostCPUGetMap() virsh-domain.c: modernize virshVcpuinfoInactive() virsh-domain.c: modernize cmdVcpuinfo() virhostcpu.c: refactor virHostCPUParseCountLinux() virhostcpu.c: introduce virHostCPUGetAvailableCPUsBitmap() conf, qemu: consider available CPUs in vcpupin/emulatorpin output src/conf/domain_conf.c | 4 +-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 10 ++- src/util/virhostcpu.c| 63 src/util/virhostcpu.h| 2 ++ tools/virsh-domain.c | 44 ++-- 6 files changed, 59 insertions(+), 65 deletions(-) -- 2.26.2
[PATCH 7/7] conf, qemu: consider available CPUs in vcpupin/emulatorpin output
The output of vcpupin and emulatorpin for a domain with vcpu
placement='static' is based on a default bitmap that contains
all possible CPUs in the host, regardless of the CPUs being offline
or not. E.g. for a Linux host with this CPU setup (from lscpu):
On-line CPU(s) list: 0,8,16,24,32,40,(...),184
Off-line CPU(s) list: 1-7,9-15,17-23,25-31,(...),185-191
And a domain with this configuration:
1
'virsh vcpupin' will return the following:
$ sudo ./run tools/virsh vcpupin vcpupin_test
VCPU CPU Affinity
--
0 0-191
This is benign by its own, but can make the user believe that all
CPUs from the 0-191 range are eligible for pinning. Which can lead
to situations like this:
$ sudo ./run tools/virsh vcpupin vcpupin_test 0 1
error: Invalid value '1' for 'cpuset.cpus': Invalid argument
This is exarcebated by the fact that 'virsh vcpuinfo' considers only
available host CPUs in the 'CPU Affinity' field:
$ sudo ./run tools/virsh vcpuinfo vcpupin_test
(...)
CPU Affinity: y---y---y---(...)
This patch changes the default bitmap of vcpupin and emulatorpin, in
the case of domains with static vcpu placement, to all available CPUs
instead of all possible CPUs. Aside from making it consistent with
the behavior of 'vcpuinfo', users will now have one less incentive to
try to pin a vcpu in an offline CPU.
https://bugzilla.redhat.com/show_bug.cgi?id=1434276
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 4 +---
src/qemu/qemu_driver.c | 7 +--
2 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 31ba78b950..6f95023aaf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2089,11 +2089,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
if (hostcpus < 0)
return -1;
-if (!(allcpumap = virBitmapNew(hostcpus)))
+if (!(allcpumap = virHostCPUGetAvailableCPUsBitmap()))
return -1;
-virBitmapSetAll(allcpumap);
-
for (i = 0; i < maxvcpus && i < ncpumaps; i++) {
virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i);
virBitmapPtr bitmap = NULL;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d486ce5278..22f0313394 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5425,7 +5425,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
virDomainDefPtr def;
bool live;
int ret = -1;
-int hostcpus;
virBitmapPtr cpumask = NULL;
g_autoptr(virBitmap) bitmap = NULL;
virBitmapPtr autoCpuset = NULL;
@@ -5442,9 +5441,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
if (!(def = virDomainObjGetOneDefState(vm, flags, )))
goto cleanup;
-if ((hostcpus = virHostCPUGetCount()) < 0)
-goto cleanup;
-
if (live)
autoCpuset = QEMU_DOMAIN_PRIVATE(vm)->autoCpuset;
@@ -5456,9 +5452,8 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
autoCpuset) {
cpumask = autoCpuset;
} else {
-if (!(bitmap = virBitmapNew(hostcpus)))
+if (!(bitmap = virHostCPUGetAvailableCPUsBitmap()))
goto cleanup;
-virBitmapSetAll(bitmap);
cpumask = bitmap;
}
--
2.26.2
[PATCH 2/7] virhostcpu.c: use g_autoptr in virHostCPUGetMap()
Signed-off-by: Daniel Henrique Barboza
---
src/util/virhostcpu.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 39bbcf8ca8..3023bca831 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1089,7 +1089,7 @@ virHostCPUGetMap(unsigned char **cpumap,
unsigned int *online,
unsigned int flags)
{
-virBitmapPtr cpus = NULL;
+g_autoptr(virBitmap) cpus = NULL;
int ret = -1;
int dummy;
@@ -,7 +,6 @@ virHostCPUGetMap(unsigned char **cpumap,
cleanup:
if (ret < 0 && cpumap)
VIR_FREE(*cpumap);
-virBitmapFree(cpus);
return ret;
}
--
2.26.2
[PATCH 4/7] virsh-domain.c: modernize cmdVcpuinfo()
Use g_auto* pointers to avoid the need for the cleanup label. The
type of the pointer 'virDomainPtr dom' was changed to its alias
'virshDomainPtr' to allow the use of g_autoptr().
Signed-off-by: Daniel Henrique Barboza
---
tools/virsh-domain.c | 26 +-
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 23d41a4ddf..cfae42eda9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6928,12 +6928,11 @@ static bool
cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
{
virDomainInfo info;
-virDomainPtr dom;
-virVcpuInfoPtr cpuinfo = NULL;
-unsigned char *cpumaps = NULL;
+g_autoptr(virshDomain) dom = NULL;
+g_autofree virVcpuInfoPtr cpuinfo = NULL;
+g_autofree unsigned char *cpumaps = NULL;
int ncpus, maxcpu;
size_t cpumaplen;
-bool ret = false;
bool pretty = vshCommandOptBool(cmd, "pretty");
int n;
virshControlPtr priv = ctl->privData;
@@ -6942,10 +6941,10 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
return false;
if ((maxcpu = virshNodeGetCPUCount(priv->conn)) < 0)
-goto cleanup;
+return false;
if (virDomainGetInfo(dom, ) != 0)
-goto cleanup;
+return false;
cpuinfo = vshMalloc(ctl, sizeof(virVcpuInfo)*info.nrVirtCpu);
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
@@ -6955,13 +6954,12 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
cpuinfo, info.nrVirtCpu,
cpumaps, cpumaplen)) < 0) {
if (info.state != VIR_DOMAIN_SHUTOFF)
-goto cleanup;
+return false;
vshResetLibvirtError();
/* for offline VMs we can return pinning information */
-ret = virshVcpuinfoInactive(ctl, dom, maxcpu, pretty);
-goto cleanup;
+return virshVcpuinfoInactive(ctl, dom, maxcpu, pretty);
}
for (n = 0; n < ncpus; n++) {
@@ -6979,19 +6977,13 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen,
n),
maxcpu, pretty) < 0)
-goto cleanup;
+return false;
if (n < (ncpus - 1))
vshPrint(ctl, "\n");
}
-ret = true;
-
- cleanup:
-VIR_FREE(cpumaps);
-VIR_FREE(cpuinfo);
-virshDomainFree(dom);
-return ret;
+return true;
}
/*
--
2.26.2
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
Hello, This patch is generating errors when booting Libvirt in a Power 8 host: 2020-06-26 21:35:49.703+: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address 2020-06-26 21:35:49.703+: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address This is not related to Power though. The check for ZPCI address is incomplete is being done prior to the check of ZPCI address is present, in virDomainDeviceInfoFormat(). I already posted a patch that moves the verification to the right code block, avoiding the errors when there is no ZPCI device in the domain: https://www.redhat.com/archives/libvir-list/2020-June/msg01261.html Thanks, DHB
[PATCH 1/1] domain_conf.c: skip checking ZPCI address is incomplete if not present
Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
prior to checking if the device has a ZPCI address at all. This
results in errors like these when starting Libvirt:
error : virDomainDeviceInfoFormat:7527 : internal error:
Missing uid or fid attribute of zPCI address
Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
check done by virZPCIDeviceAddressIsPresent().
Fixes: 076591009ad11ec108521b52a4945d0f895fa160
CC: Bjoern Walk
CC: Boris Fiuczynski
CC: Shalini Chellathurai Saroja
CC: Andrea Bolognani
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 31ba78b950..33f177b16f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7522,11 +7522,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
-if (virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing uid or fid attribute of zPCI address"));
-}
if (virZPCIDeviceAddressIsPresent(>addr.pci.zpci)) {
+if (virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci))
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing uid or fid attribute of zPCI
address"));
+
virBufferAsprintf(,
"\n",
info->addr.pci.zpci.uid.value,
--
2.26.2
Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
On 6/26/20 3:22 AM, Peter Krempa wrote:
> On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
>> When executing the hypervisor-cpu-compare/baseline commands and
>> the XML file contains a CPU definition using host-passthrough
>> and no model name, the commands will fail and return an error
>> message from the QMP response.
>
> This kind of logic does not seem to belong ...
>
>> Let's fix this by checking for host-passthrough and a missing
>> model name when converting a CPU definition to a JSON object.
>> If these conditions are matched, then the JSON object will be
>> constructed using "host" as the model name.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
>>
>> Signed-off-by: Collin Walling
>> ---
>> src/qemu/qemu_monitor_json.c | 13 -
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 3070c1e6b3..448a3a9356 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
>> {
>> virJSONValuePtr model = virJSONValueNewObject();
>> virJSONValuePtr props = NULL;
>> +const char *model_name = cpu->model;
>> size_t i;
>>
>> -if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
>> +if (!model_name) {
>> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>> +model_name = "host";
>> +} else {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("cpu parameter is missing a model name"));
>> +goto error;
>> +}
>> +}
>
> ... to the monitor code, where we try to just deal with the monitor
> itself rather than hiding any logic.
>
That's fair. Perhaps it should belong to the qemu driver code after the
CPU XML to a CPU def conversion. I'm also unsure if "host" is an s390
specific thing, or if other archs use it as well.
> But the final word needs to be from Jirka, but he is on holidays until
> the end of next week.
>
>
>> +
>> +if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
>> goto error;
>>
>> if (cpu->nfeatures || !migratable) {
>> --
>> 2.26.2
>>
>
--
Regards,
Collin
Stay safe and stay healthy
[libvirt PATCH] ci: Run all jobs, for all branches, all the time
After recent changes (increasing the parallelism of the pipeline by reducing the number of stages, introducing FreeBSD builds that take longer than any other job), the difference between running the full pipeline or a reduced one has basically disappeared: in both cases, the completion time is around 25-35 minutes depending on whether containers need to be rebuilt and how many shared runners are available. Reduce the complexity of our .gitlab-ci.yml and make things simpler for contributors by simply always running all jobs. Signed-off-by: Andrea Bolognani --- .gitlab-ci.yml | 143 ++--- 1 file changed, 53 insertions(+), 90 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6cb910b0fa..e6eb2f9905 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -15,8 +15,7 @@ stages: # Common templates -# Containers that are necessary for the default pipeline -.container_default_job_template: _default_job_definition +.container_job_template: _job_definition image: docker:stable stage: containers services: @@ -33,23 +32,15 @@ stages: after_script: - docker logout -# Containers that are only needed for the full pipeline -.container_extra_job_template: _extra_job_definition - <<: *container_default_job_definition - only: -- master -- /^ci-full-.*$/ - # We build many containers which can be useful to debug problems but are not # needed for the pipeline itself to complete: those sometimes fail, and when # that happens it's mostly because of temporary issues with Debian sid. We # don't want those failures to affect the overall pipeline status .container_optional_job_template: _optional_job_definition - <<: *container_extra_job_definition + <<: *container_job_definition allow_failure: true -# Default native build jobs that are always run -.native_build_default_job_template: _build_default_job_definition +.native_build_job_template: _build_job_definition stage: builds image: $CI_REGISTRY_IMAGE/ci-$NAME:latest cache: @@ -64,18 +55,10 @@ stages: - ../autogen.sh || (cat config.log && exit 1) - $MAKE distcheck -# Extra native build jobs that are only run post-merge, or -# when code is pushed to a branch with "ci-full-" name prefix -.native_build_extra_job_template: _build_extra_job_definition - <<: *native_build_default_job_definition - only: -- master -- /^ci-full-.*$/ - # Jobs that we delegate to Cirrus CI because they require an operating # system other than Linux. These jobs will only run if the required # setup has been performed on the GitLab account (see ci/README.rst). -.cirrus_build_default_job_template: _build_default_job_definition +.cirrus_build_job_template: _build_job_definition stage: builds image: registry.gitlab.com/libvirt/libvirt-ci/cirrus-run:master script: @@ -85,19 +68,7 @@ stages: - $CIRRUS_GITHUB_REPO - $CIRRUS_API_TOKEN -.cirrus_build_extra_job_template: _build_extra_job_definition - <<: *cirrus_build_default_job_definition - only: -variables: - - $CIRRUS_GITHUB_REPO - - $CIRRUS_API_TOKEN -refs: - - master - - /^ci-full-.*$/ - - -# Default cross build jobs that are always run -.cross_build_default_job_template: _build_default_job_definition +.cross_build_default_job_template: _build_job_definition stage: builds image: $CI_REGISTRY_IMAGE/ci-$NAME-cross-$CROSS:latest cache: @@ -112,74 +83,66 @@ stages: - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1) - $MAKE -# Extra cross build jobs that are only run post-merge, or -# when code is pushed to a branch with "ci-full-" name prefix -.cross_build_extra_job_template: _build_extra_job_definition - <<: *cross_build_default_job_definition - only: -- master -- /^ci-full-.*$/ - # Native container build jobs x64-centos-7-container: - <<: *container_default_job_definition + <<: *container_job_definition variables: NAME: centos-7 x64-centos-8-container: - <<: *container_default_job_definition + <<: *container_job_definition variables: NAME: centos-8 x64-centos-stream-container: - <<: *container_extra_job_definition + <<: *container_job_definition variables: NAME: centos-stream x64-debian-9-container: - <<: *container_extra_job_definition + <<: *container_job_definition variables: NAME: debian-9 x64-debian-10-container: - <<: *container_default_job_definition + <<: *container_job_definition variables: NAME: debian-10 x64-debian-sid-container: - <<: *container_extra_job_definition + <<: *container_job_definition variables: NAME: debian-sid x64-fedora-31-container: - <<: *container_extra_job_definition + <<: *container_job_definition variables: NAME: fedora-31 x64-fedora-32-container: - <<: *container_default_job_definition + <<: *container_job_definition variables: NAME: fedora-32 x64-fedora-rawhide-container: - <<:
Re: [PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390
On Fri, 2020-06-26 at 14:13 +0200, Shalini Chellathurai Saroja wrote:
> On 6/25/20 8:01 PM, Andrea Bolognani wrote:
> > Aside from the comments in patch 2/5, everything looks good to me.
> >
> > The series does, however, not update the release notes (NEWS.rst):
> > can you please post a 6/5 patch that takes care of that, assuming we
> > decide not to go with a respin?
>
> Hi Andrea,
>
> Thank you :-). I have attached the patch to update the release notes
> with this email.
Thanks. I tweaked the commit message a bit, and I also had to squash
the diff below into patch 2 to make CentOS 7 and macOS happy. The
series is now pushed. Have a nice weekend :)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 21398e90f2..64a713a5f9 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -52,7 +52,7 @@ static int
virZPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
{
-virZPCIDeviceAddress def = { 0 };
+virZPCIDeviceAddress def = { .uid = { 0 }, .fid = { 0 } };
g_autofree char *uid = NULL;
g_autofree char *fid = NULL;
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
On 6/25/20 11:12 AM, Daniel P. Berrangé wrote:
On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 59 +
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
goto error;
In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.
In general, mass replacements should be done only to
g_clear_pointer(, g_free)
and I'm not sure it's worth it.
There's no getting around it - that looks ugly. And who wants to replace
5506 occurences of one simple-looking thing with something else that's
functionally equivalent but more painful to look at?
I would vote for just documenting that, for safety and consistency reasons,
VIR_FREE() should always be used instead of g_free(), and eliminating all
direct use of g_free() (along with the aforementioned syntax check). (BTW, I
had assumed there had been more changes to g_free(), but when I looked at my
current tree just now, there were only 228 occurences, including the changes
in this patch)
The point in getting rid of VIR_FREE is so that we reduce the libvirt
specific wrappers in favour of standard APIs.
Is this just to make the code more accessible/understandable to new
contributors? Or is there some other reason that I missed due to being
incapable of reading all the messages on all the lists? (I guess there's
also the issue of reducing support burden by reproducing identical code
to something that someone else is already maintaining in a different
library. But in this case we're just talking about a few lines that
enforces good behavior.)
A large portion of the VIR_FREE's will be eliminated by g_autoptr.
Another large set of them are used in the virFooStructFree() methods.
Those can all be converted to g_free safely, as all the methods do
is free stuff.
Most VIR_FREEs that occur at the exit of functions can also be
safely converted to g_free, if g_autoptr isnt applicable. Sometimes
needs a little care if you have multiple goto jumps between labels.
It still requires thought + diligence = time. And what if new code is
added to the end of a function, thus making those things that had been
"at the end" now in the middle. The more thought and decision making is
needed to get something right, the more likely it is that someone will
get it wrong.
The big danger cases are the VIR_FREE()s that occur in the middle
of methods, especially in loop bodies. Those the ones that must
use the g_clear_pointer, and that's not very many of those, so the
ugly syntax isn't an issue.
1) Maybe I'll feel differently after more of the code has been converted
to use g_auto* and eliminated more of the existing explicit frees, but
with currently > 5000 uses of VIR_FREE still in the code, I fear that
"not many of those" may be more than we're expecting, and especially
with many of them left, it would give me more warm fuzzies to be able to
say
"We can verifiably state that no pointers will be used
after free , because their values have been NULLed,
and any access will either be a NOP, or cause an
immediate segfault"
rather than
"We believe that the contributors to libvirt have been
diligent in their manual auditing of all cases of
free'ing memory to assure that none of the freed
pointers are ever used at any later point,
because well, just *because*".
(on the other hand, admittedly any pointer to something with its own
vir*Free() function already requires diligence on the part of the
programmer, since vir*Free() doesn't NULL the pointer. In that case,
what's a little extra burden?)
2) Speaking from my experience with the occurrences I converted here,
the worst offenders were the places where someone re-used a local
pointer multiple times in a function (sometimes naming the multiply-used
variable something generic like "tmp", other times naming it
specifically (e.g. "flags", then using it once for a matching purpose
(e.g. a string containing the flags arg for an ebtables command option),
and again for something only tangentially related (e.g. the *mask* arg
for an ebtables command
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On Fri, 2020-06-26 at 16:08 +0200, Bjoern Walk wrote:
> Andrea Bolognani [2020-06-25, 07:43PM +0200]:
> > static int
> > virDomainZPCIAddressReserveId(virHashTablePtr set,
> > - unsigned int id,
> > + virZPCIDeviceAddressID *id,
> >const char *name)
> > {
> > -if (virHashLookup(set, )) {
> > +if (!id->isSet) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("No zPCI %s to reserve"),
> > + name);
>
> Maybe it's better to fail silently here (or not fail at all and just
> make it no-op)? This is more of an assert that concerns the developer
> and not something the user can understand.
VIR_ERR_INTERNAL_ERROR is commonly used when encountering situations
that Will Never Happen™.
> > static void
> > virDomainZPCIAddressReleaseId(virHashTablePtr set,
> > - unsigned int *id,
> > + virZPCIDeviceAddressID *id,
> >const char *name)
> > {
> > -if (virHashRemoveEntry(set, id) < 0) {
> > +if (!id->isSet)
> > +return;
> > +
> > +if (virHashRemoveEntry(set, >value) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Release %s %o failed"),
> > - name, *id);
> > + name, id->value);
> > }
> >
> > -*id = 0;
> > +id->value = 0;
> > +id->isSet = false;
>
> Not sure if I don't want to leave this to the caller. 99% of time it
> shouldn't matter because the released device is deleted from the domain
> anyways, but this tight coupling would prevent hypothetical re-use of
> the id.
id->value is zeroed anyway, so there's not much re-using that can
happen. If you're not deleting the device, then you're going to call
virDomainZPCIAddressAssign{U,F}id() next, and those expect id->isSet
to be false.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
Andrea Bolognani [2020-06-25, 07:43PM +0200]:
> From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001
> From: Andrea Bolognani
> Date: Thu, 25 Jun 2020 18:37:27 +0200
> Subject: [libvirt PATCH 1/2] conf: Move id->{value,isSet} handling further
> down the stack
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/domain_addr.c | 61 --
> 1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 90ed31ef4e..493b155129 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -33,20 +33,27 @@ VIR_LOG_INIT("conf.domain_addr");
>
> static int
> virDomainZPCIAddressReserveId(virHashTablePtr set,
> - unsigned int id,
> + virZPCIDeviceAddressID *id,
>const char *name)
> {
> -if (virHashLookup(set, )) {
> +if (!id->isSet) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No zPCI %s to reserve"),
> + name);
Maybe it's better to fail silently here (or not fail at all and just
make it no-op)? This is more of an assert that concerns the developer
and not something the user can understand.
> +return -1;
> +}
> +
> +if (virHashLookup(set, >value)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("zPCI %s %o is already reserved"),
> - name, id);
> + name, id->value);
> return -1;
> }
>
> -if (virHashAddEntry(set, , (void*)1) < 0) {
> +if (virHashAddEntry(set, >value, (void*)1) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to reserve %s %o"),
> - name, id);
> + name, id->value);
> return -1;
> }
>
> [...]
>
> static void
> virDomainZPCIAddressReleaseId(virHashTablePtr set,
> - unsigned int *id,
> + virZPCIDeviceAddressID *id,
>const char *name)
> {
> -if (virHashRemoveEntry(set, id) < 0) {
> +if (!id->isSet)
> +return;
> +
> +if (virHashRemoveEntry(set, >value) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Release %s %o failed"),
> - name, *id);
> + name, id->value);
> }
>
> -*id = 0;
> +id->value = 0;
> +id->isSet = false;
Not sure if I don't want to leave this to the caller. 99% of time it
shouldn't matter because the released device is deleted from the domain
anyways, but this tight coupling would prevent hypothetical re-use of
the id.
Reviewed-by: Bjoern Walk
--
IBM Systems
Linux on Z & Virtualization Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
signature.asc
Description: PGP signature
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
First of all, this is much closer to what I had in mind. Good job!
Hi Andrea,
Thank you:-)
We're not quite there yet, though: as you can see from the hunks
above, there are still many scenarios in which we're either
manipulating id->value and id->isSet separately, or we needlessly
duplicate checks and don't take advantage of the fact that we now
have the virZPCIDeviceAddressID struct.
I have attached a patch that fixes these issues: as you can see,
it's pretty simple, because you've laid all the groundwork:) so it
just needs to polish things up a bit. It also solves the situation
where calling virDomainZPCIAddressRelease{U,F}id() would not set
id->isSet back to false.
Speaking of polish, while functionally this doesn't make any
difference it would be IMHO better to rename the current
virDomainZPCIAddressReserveAddr() to
virDomainZPCIAddressEnsureAddr(), since in terms of semantics it
more closely matches those of the PCI address function of the same
name. This will hopefully help reduce confusion. I've attached a
patch that performs this change as well.
Everything else looks good. Please let me know what you think of
the changes in the attached patches!
As Boris has already mentioned, these patches provide a much better code,
thank you:-)
Re: [PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390
On 6/25/20 8:01 PM, Andrea Bolognani wrote: On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote: Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device Aside from the comments in patch 2/5, everything looks good to me. The series does, however, not update the release notes (NEWS.rst): can you please post a 6/5 patch that takes care of that, assuming we decide not to go with a respin? Hi Andrea, Thank you :-). I have attached the patch to update the release notes with this email. Thanks! >From 24627aba97467f2c3e4ccf234faf294007c15435 Mon Sep 17 00:00:00 2001 From: Shalini Chellathurai Saroja Date: Fri, 26 Jun 2020 10:59:41 +0200 Subject: [PATCH libvirt v2 6/5] news: documentation of fix Signed-off-by: Shalini Chellathurai Saroja --- NEWS.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 54b1e4a9..0e9822cd 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -56,6 +56,11 @@ v6.5.0 (unreleased) This release fixes a regression which was introduced in libvirt v6.4.0 where libvirtd always crashes when a block commit of a disk is requested. + * qemu: fixed zPCI address auto generation on s390 + +Removes the correlation between the zPCI address attributes uid and fid. +Fixes the validation and autogeneration of zPCI address attributes. + v6.4.0 (2020-06-02) === -- 2.25.4
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
@@ -129,7 +129,8 @@ static void
virDomainZPCIAddressReleaseUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
-virDomainZPCIAddressReleaseId(set, >uid, "uid");
+if (addr->uid.isSet)
+virDomainZPCIAddressReleaseId(set, >uid.value, "uid");
}
@@ -137,7 +138,8 @@ static void
virDomainZPCIAddressReleaseFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
-virDomainZPCIAddressReleaseId(set, >fid, "fid");
+if (addr->fid.isSet)
+virDomainZPCIAddressReleaseId(set, >fid.value, "fid");
}
-static int
-virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
-virZPCIDeviceAddressPtr addr)
-{
-if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
-return -1;
-
-if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
-virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
-return -1;
-}
+addr->uid.isSet = true;
+addr->fid.isSet = true;
First of all, this is much closer to what I had in mind. Good job!
We're not quite there yet, though: as you can see from the hunks
above, there are still many scenarios in which we're either
manipulating id->value and id->isSet separately, or we needlessly
duplicate checks and don't take advantage of the fact that we now
have the virZPCIDeviceAddressID struct.
I have attached a patch that fixes these issues: as you can see,
it's pretty simple, because you've laid all the groundwork :) so it
just needs to polish things up a bit. It also solves the situation
where calling virDomainZPCIAddressRelease{U,F}id() would not set
id->isSet back to false.
Speaking of polish, while functionally this doesn't make any
difference it would be IMHO better to rename the current
virDomainZPCIAddressReserveAddr() to
virDomainZPCIAddressEnsureAddr(), since in terms of semantics it
more closely matches those of the PCI address function of the same
name. This will hopefully help reduce confusion. I've attached a
patch that performs this change as well.
Everything else looks good. Please let me know what you think of
the changes in the attached patches!
Hi Andrea,
I agree that it looks nice and sparkling. :D Thanks.
Reviewed-by: Boris Fiuczynski
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [libvirt PATCH v2] ci: Use openSUSE image for code style checking
On Thu, Jun 25, 2020 at 03:43:37PM -0500, Jonathon Jongsma wrote: > On Mon, 2020-06-22 at 18:18 +0200, Andrea Bolognani wrote: > > On Mon, 2020-06-22 at 09:43 -0500, Jonathon Jongsma wrote: > > > CentOS does not have the cppi package, so some code style checks > > > are > > > skipped. Switch to a openSUSE image to do the code style checks. > > > > > > Signed-off-by: Jonathon Jongsma > > > --- > > > .gitlab-ci.yml | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Reviewed-by: Andrea Bolognani > > > > Not that it's a particularly important patch, but I just thought I'd > mention that if we do want it upstream, I'll need somebody to push it > for me ;) Reviewed-by: Erik Skultety and pushed. Regards, Erik
Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
> When executing the hypervisor-cpu-compare/baseline commands and
> the XML file contains a CPU definition using host-passthrough
> and no model name, the commands will fail and return an error
> message from the QMP response.
This kind of logic does not seem to belong ...
> Let's fix this by checking for host-passthrough and a missing
> model name when converting a CPU definition to a JSON object.
> If these conditions are matched, then the JSON object will be
> constructed using "host" as the model name.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
>
> Signed-off-by: Collin Walling
> ---
> src/qemu/qemu_monitor_json.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3070c1e6b3..448a3a9356 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
> {
> virJSONValuePtr model = virJSONValueNewObject();
> virJSONValuePtr props = NULL;
> +const char *model_name = cpu->model;
> size_t i;
>
> -if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
> +if (!model_name) {
> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +model_name = "host";
> +} else {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cpu parameter is missing a model name"));
> +goto error;
> +}
> +}
... to the monitor code, where we try to just deal with the monitor
itself rather than hiding any logic.
But the final word needs to be from Jirka, but he is on holidays until
the end of next week.
> +
> +if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
> goto error;
>
> if (cpu->nfeatures || !migratable) {
> --
> 2.26.2
>
