[PATCH for 6.5.0 0/3] Move zPCI validation from formatter to validator

*** BLURB HERE *** Michal Prívozník (3): qemuhotplugtest: Free monitor iff successfully initialized domain_conf: Move zPCI validation from formatter to validator qemu_validate: Fix how qemuValidateDomainDeviceDefZPCIAddress() is called src/conf/domain_conf.c | 4 ---- src/qemu/qemu_validate.c | 9 ++++++++- tests/qemuhotplugtest.c | 8 +++++--- 3 files changed, 13 insertions(+), 8 deletions(-) -- 2.26.2

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@redhat.com> --- 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

On Mon, 2020-06-29 at 09:43 +0200, Michal Privoznik wrote:
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@redhat.com> --- tests/qemuhotplugtest.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> and safe for freeze. -- Andrea Bolognani / Red Hat / Virtualization

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@redhat.com> --- 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 Mon, 2020-06-29 at 09:43 +0200, Michal Privoznik wrote:
+++ 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; + }
Throwing an XML_ERROR here doesn't seem right, because from the user's point of view it's perfectly fine to only provide a partial zPCI address: by the point we get here, we've already gone through the PostParse callback, so if either attribute is missing that's because of a developer mistake. Do we even need this check at all? I'm the one who asked for it to be introduced, but now I'm questioning that :) -- Andrea Bolognani / Red Hat / Virtualization

On 6/29/20 11:58 AM, Andrea Bolognani wrote:
On Mon, 2020-06-29 at 09:43 +0200, Michal Privoznik wrote:
+++ 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; + }
Throwing an XML_ERROR here doesn't seem right, because from the user's point of view it's perfectly fine to only provide a partial zPCI address: by the point we get here, we've already gone through the PostParse callback, so if either attribute is missing that's because of a developer mistake.
Do we even need this check at all? I'm the one who asked for it to be introduced, but now I'm questioning that :)
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

On Mon, 2020-06-29 at 12:01 +0200, Michal Privoznik wrote:
On 6/29/20 11:58 AM, Andrea Bolognani wrote:
On Mon, 2020-06-29 at 09:43 +0200, Michal Privoznik wrote:
+++ 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; + }
Throwing an XML_ERROR here doesn't seem right, because from the user's point of view it's perfectly fine to only provide a partial zPCI address: by the point we get here, we've already gone through the PostParse callback, so if either attribute is missing that's because of a developer mistake.
Do we even need this check at all? I'm the one who asked for it to be introduced, but now I'm questioning that :)
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?
I think so, yes. If someone finds a flaw in my reasoning, we still have a week to change it back. -- Andrea Bolognani / Red Hat / Virtualization

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@redhat.com> --- 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

On Mon, 2020-06-29 at 09:43 +0200, Michal Privoznik wrote:
+++ 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;
There needs to be a 'break' after the 'if', otherwise on success this case will fall through to the next case, which is not what we want. With that added, Reviewed-by: Andrea Bolognani <abologna@redhat.com> and safe for freeze. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani
-
Michal Privoznik