Re: [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 03/11/2018 08:13 PM, Jim Fehlig wrote:
> On 03/09/2018 09:47 AM, John Ferlan wrote:
> > Commit id '9ac945078' altered libxlDomObjFromDomain to return
> > a locked *and* ref counted object for some specific purposes;
> > however, it neglected to alter all the consumers of the helper
> > to use virDomainObjEndAPI thus leaving many objects with extra
> > ref counts.
> >
> > The two consumers for libxlDomainMigrationConfirm would also
> > originally use the libxlDomObjFromDomain API (either from
> > libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
> > libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
> >
> > Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> > ---
> >   src/libxl/libxl_driver.c    | 86 ++++++++++++++++-----------------------------
> >   src/libxl/libxl_migration.c |  3 +-
> >   2 files changed, 31 insertions(+), 58 deletions(-)
> >
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index b5101626e..9aa4a293c 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
> > flags)
> >       }
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
> >       }
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom)
> >           goto cleanup;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return type;
> >   }
> > @@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom)
> >       ret = virDomainDefGetMemoryTotal(vm->def);
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom,
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, 
> > unsigned int flags)
> >       ret = vm->hasManagedSave;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned 
> > int flags)
> >    cleanup:
> >       VIR_FREE(name);
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int 
> > flags)
> >           ret = virDomainDefGetVcpus(def);
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
> >                                              libxl_get_max_cpus(cfg->ctx), NULL);
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr 
> > info, int maxinfo,
> >       ret = maxinfo;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
> >                                virDomainDefFormatConvertXMLFlags(flags));
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
> >    cleanup:
> >       VIR_FREE(name);
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       if (event)
> >           libxlDomainEventQueue(driver, event);
> >       virObjectUnref(cfg);
> > @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const 
> > char *xml,
> >    cleanup:
> >       virDomainDefFree(vmdef);
> >       virDomainDeviceDefFree(dev);
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart)
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
> >       ignore_value(VIR_STRDUP(ret, name));
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom,
> >       }
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom,
> >       VIR_FREE(nodeset);
> >       virBitmapFree(nodes);
> >       libxl_bitmap_dispose(&nodemap);
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       virObjectUnref(cfg);
> >       return ret;
> >   }
> > @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom)
> >       ret = virDomainObjIsActive(obj);
> >    cleanup:
> > -    if (obj)
> > -        virObjectUnlock(obj);
> > +    virDomainObjEndAPI(&obj);
> >       return ret;
> >   }
> > @@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom)
> >       ret = obj->persistent;
> >    cleanup:
> > -    if (obj)
> > -        virObjectUnlock(obj);
> > +    virDomainObjEndAPI(&obj);
> >       return ret;
> >   }
> > @@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom)
> >       ret = vm->updated;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom,
> >                                             start_cpu, ncpus);
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom,
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
> > @@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom,
> >       ret = 0;
> >    cleanup:
> > -    if (vm)
> > -        virObjectUnlock(vm);
> > +    virDomainObjEndAPI(&vm);
> >       return ret;
> >   }
>
> For these changes
>
> Reviewed-by: Jim Fehlig <jfehlig@xxxxxxxx>

I've pushed this part of the patch. Your fixes have encouraged me to audit all 
the begin/end API code in the libxl driver and I'd like to base any fixes on top 
of items we've already hashed out.

Regards,
Jim

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