This allows us to simplify the function and avoid jumping to 'cleanup'.

Signed-off-by: Jonathon Jongsma <jjongsma@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;
 
     if ((ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps)) < 0)
-        goto cleanup;
+        return ret;
 
     if (virDomainCapsDeviceDefValidate(domCaps, dev, def) < 0)
-        goto cleanup;
+        return ret;
 
     switch ((virDomainDeviceType)dev->type) {
     case VIR_DOMAIN_DEVICE_NET:
@@ -7300,9 +7300,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
         break;
     }
 
- cleanup:
-    virObjectUnref(qemuCaps);
-    virObjectUnref(domCaps);
     return ret;
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list