Re: [PATCH v2 2/6] qemu: use VIR_AUTOUNREF in qemuDomainDeviceDefValidate()

[Cole Robinson <crobinso@xxxxxxxxxx>]

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