Re: [libvirt] [PATCH v2 2/6] qemu: use VIR_AUTOUNREF in qemuDomainDeviceDefValidate()
On 10/11/19 5:27 PM, Jonathon Jongsma wrote:
This allows us to simplify the function and avoid jumping to
'cleanup'.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_domain.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dc7568fe18..516ae7e444 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7197,8 +7197,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
{
int ret = 0;
virQEMUDriverPtr driver = opaque;
- virQEMUCapsPtr qemuCaps = NULL;
- virDomainCapsPtr domCaps = NULL;
+ VIR_AUTOUNREF(virQEMUCapsPtr) qemuCaps = NULL;
+ VIR_AUTOUNREF(virDomainCapsPtr) domCaps = NULL;
if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
def->emulator)))
@@ -7208,13 +7208,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
def->os.machine,
def->os.arch,
def->virtType)))
- goto cleanup;
+ return -1;
This patch actually uncovers a bug in the domcaps caching,
qemumemlocktest is failing. I sent a fix now and CCd you. Previously
even when this function failed, we would return 0; So this patch has a
side effect besides the cleanup.
I think you should move patch #3 before this and have it contain all the
bug fixing here. Then do the VIR_AUTOFREE cleanup on top, which should
be a no-op. Without cleaning up the function, the bug fixing will look a
bit weird:
if (!(qemuCaps = virQEMUBlah())) {
ret = -1;
goto cleanup;
}
That's due to this function being a little funky because it's using
'ret' as both the function return code, and to capture the return value
of functions we are calling. Elsewhere in libvirt we will use a separate
'int rc' to capture return values. Then the code is more like:
int ret = -1;
int rc;
if (foo() < 0)
goto cleanup;
if ((rc = bar()) < 0) {
ret = rc;
goto cleanup;
}
switch()...
ret = rc;
cleanup:
return ret;
But after the VIR_AUTOFREE cleanup that distinction probably doesn't
matter, so if the bug fix is a bit ugly than IMO that's fine because the
VIR_AUTOFREE conversion will clean it up anyways
But, did you run the test suite on this series? There are the errors I
hit here, but there's also tons of qemuxml2xml breakage with caused by
the video validation move. Please make sure you run 'make check' and
'make syntax-check' on every commit in a series before sending. I'll
reply to the other patches with more info.
Thanks,
Cole