If initializing test monitor in testQemuHotplugCpuPrepare() fails, the control jumps to error label where testQemuHotplugCpuDataFree() is called. But since the data->mon is NULL due to aforementioned failure, qemuMonitorTestGetMonitor() dereferences a NULL pointer leading to a SIGSEGV. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/qemuhotplugtest.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index ba3fc4d814..ba30cf5aa6 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -399,9 +399,11 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) virObjectUnref(data->vm); } - mon = qemuMonitorTestGetMonitor(data->mon); - virObjectLock(mon); - qemuMonitorTestFree(data->mon); + if (data->mon) { + mon = qemuMonitorTestGetMonitor(data->mon); + virObjectLock(mon); + qemuMonitorTestFree(data->mon); + } VIR_FREE(data); } -- 2.26.2

In 076591009ad a validation code was added to virDomainDeviceInfoFormat() which reports an error if zPCI address entered in was incomplete. But, there are two problems with this approach. The first problem is the placement of the code - it doesn't belong into XML formatter rather than XML validator. The second one is that the code doesn't check if the PCI address it's validating has zPCI extension and thus the error is reported for normal PCI addresses too, hence the change of the function that's being called. The second problem is addressed in d482cf6bef which doesn't address the first problem. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 4 ---- src/qemu/qemu_validate.c | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33f177b16f..0c883cd834 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7523,10 +7523,6 @@ virDomainDeviceInfoFormat(virBufferPtr buf, } if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) { - if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing uid or fid attribute of zPCI address")); - virBufferAsprintf(&childBuf, "<zpci uid='0x%.4x' fid='0x%.8x'/>\n", info->addr.pci.zpci.uid.value, diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 78efa68584..2d3a2ec93a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1028,6 +1028,12 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfoPtr info, return -1; } + if (virDeviceInfoPCIAddressExtensionIsWanted(info)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing uid or fid attribute of zPCI address")); + return -1; + } + /* We don't need to check fid because fid covers * all range of uint32 type. */ -- 2.26.2

On 6/29/20 11:58 AM, Andrea Bolognani wrote: Maybe we don't need it then. Regardless, it doesn't belong into XML formatter. Should I post a v2 where the check is removed? Michal

To make the code future proof, the rest of the qemuValidateDomainDeviceDefAddress() has to be executed (even though there is nothing there yet) instead of returning directly. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_validate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 2d3a2ec93a..ed1497c425 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1063,7 +1063,8 @@ qemuValidateDomainDeviceDefAddress(const virDomainDeviceDef *dev, switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - return qemuValidateDomainDeviceDefZPCIAddress(info, qemuCaps); + if (qemuValidateDomainDeviceDefZPCIAddress(info, qemuCaps) < 0) + return -1; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: /* Address validation might happen before we have had a chance to -- 2.26.2