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

[Cole Robinson]

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

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

[Jonathon Jongsma]

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