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
> ---
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list