Re: [PATCH 06/20] openvz: Create accessors to virDomainObjListFindByUUID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/29/2018 01:36 PM, Jim Fehlig wrote:
> On 03/09/2018 09:48 AM, John Ferlan wrote:
>> Rather than repeat code throughout, create and use a couple of
>> accessors in order to lookup by UUID.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>   src/openvz/openvz_driver.c | 266
>> +++++++++++++--------------------------------
>>   1 file changed, 76 insertions(+), 190 deletions(-)
>>
>> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
>> index a211c370e..167ba2f7a 100644
>> --- a/src/openvz/openvz_driver.c
>> +++ b/src/openvz/openvz_driver.c
>> @@ -87,6 +87,39 @@ static void openvzDriverUnlock(struct openvz_driver
>> *driver)
>>     struct openvz_driver ovz_driver;
>>   +
>> +static virDomainObjPtr
>> +openvzDomObjFromDomainLocked(struct openvz_driver *driver,
>> +                             const unsigned char *uuid)
>> +{
>> +    virDomainObjPtr vm;
>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>> +
>> +    if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) {
>> +        virUUIDFormat(uuid, uuidstr);
>> +
>> +        virReportError(VIR_ERR_NO_DOMAIN,
>> +                       _("no domain with matching uuid '%s'"), uuidstr);
>> +        return NULL;
>> +    }
>> +
>> +    return vm;
>> +}
>> +
>> +
>> +static virDomainObjPtr
>> +openvzDomObjFromDomain(struct openvz_driver *driver,
>> +                       const unsigned char *uuid)
>> +{
>> +    virDomainObjPtr vm;
>> +
>> +    openvzDriverLock(driver);
>> +    vm = openvzDomObjFromDomainLocked(driver, uuid);
>> +    openvzDriverUnlock(driver);
>> +    return vm;
>> +}
>> +
>> +
>>   static int
>>   openvzDomainDefPostParse(virDomainDefPtr def,
>>                            virCapsPtr caps ATTRIBUTE_UNUSED,
>> @@ -281,15 +314,8 @@ openvzDomainGetHostname(virDomainPtr dom,
>> unsigned int flags)
>>       virDomainObjPtr vm;
>>         virCheckFlags(0, NULL);
>> -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return NULL;
> 
> It looks like the cleanup label can be removed from this function too.
> 

True, but it's not "as clean" as others since error does the goto clean
thing. The others where the cleanup: label disappears is because there
was only one goto cleanup... and the "if (vm)" would be useless too, but
that's handled in a couple of patches anyway.

In any case, I'll add an adjustment with my eventual followup series
that alters virDomainObjAdd processing.

>>         hostname = openvzVEGetStringParam(dom, "hostname");
>>       if (hostname == NULL)
>> @@ -359,18 +385,11 @@ static char *openvzDomainGetOSType(virDomainPtr
>> dom)
>>       virDomainObjPtr vm;
>>       char *ret = NULL;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, NULL);
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return NULL;
>>         ignore_value(VIR_STRDUP(ret,
>> virDomainOSTypeToString(vm->def->os.type)));
>>   - cleanup:
>>       if (vm)
>>           virObjectUnlock(vm);
>>       return ret;
>> @@ -384,18 +403,11 @@ static virDomainPtr
>> openvzDomainLookupByUUID(virConnectPtr conn,
>>       virDomainObjPtr vm;
>>       virDomainPtr dom = NULL;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, NULL);
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, uuid)))
>> +        return NULL;
>>         dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
>> vm->def->id);
>>   - cleanup:
>>       if (vm)
>>           virObjectUnlock(vm);
>>       return dom;
>> @@ -432,15 +444,8 @@ static int openvzDomainGetInfo(virDomainPtr dom,
>>       int state;
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (openvzGetVEStatus(vm, &state, NULL) == -1)
>>           goto cleanup;
>> @@ -480,19 +485,11 @@ openvzDomainGetState(virDomainPtr dom,
>>         virCheckFlags(0, -1);
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         ret = openvzGetVEStatus(vm, state, reason);
>>   - cleanup:
>>       if (vm)
>>           virObjectUnlock(vm);
>>       return ret;
>> @@ -505,16 +502,11 @@ static int openvzDomainIsActive(virDomainPtr dom)
>>       virDomainObjPtr obj;
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    obj = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -    if (!obj) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, NULL);
>> -        goto cleanup;
>> -    }
>> +    if (!(obj = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>> +
>>       ret = virDomainObjIsActive(obj);
>>   - cleanup:
>>       if (obj)
>>           virObjectUnlock(obj);
>>       return ret;
>> @@ -527,16 +519,11 @@ static int openvzDomainIsPersistent(virDomainPtr
>> dom)
>>       virDomainObjPtr obj;
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    obj = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -    if (!obj) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, NULL);
>> -        goto cleanup;
>> -    }
>> +    if (!(obj = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>> +
>>       ret = obj->persistent;
>>   - cleanup:
>>       if (obj)
>>           virObjectUnlock(obj);
>>       return ret;
>> @@ -554,20 +541,12 @@ static char *openvzDomainGetXMLDesc(virDomainPtr
>> dom, unsigned int flags) {
>>         /* Flags checked by virDomainDefFormat */
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return NULL;
>>         ret = virDomainDefFormat(vm->def, driver->caps,
>>                                virDomainDefFormatConvertXMLFlags(flags));
>>   - cleanup:
>>       if (vm)
>>           virObjectUnlock(vm);
>>       return ret;
>> @@ -600,15 +579,8 @@ static int openvzDomainSuspend(virDomainPtr dom)
>>       const char *prog[] = {VZCTL, "--quiet", "chkpnt",
>> PROGRAM_SENTINEL, "--suspend", NULL};
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (!virDomainObjIsActive(vm)) {
>>           virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> @@ -638,15 +610,8 @@ static int openvzDomainResume(virDomainPtr dom)
>>       const char *prog[] = {VZCTL, "--quiet", "chkpnt",
>> PROGRAM_SENTINEL, "--resume", NULL};
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (!virDomainObjIsActive(vm)) {
>>           virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> @@ -681,15 +646,8 @@ openvzDomainShutdownFlags(virDomainPtr dom,
>>         virCheckFlags(0, -1);
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (openvzGetVEStatus(vm, &status, NULL) == -1)
>>           goto cleanup;
>> @@ -744,15 +702,8 @@ static int openvzDomainReboot(virDomainPtr dom,
>>         virCheckFlags(0, -1);
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (openvzGetVEStatus(vm, &status, NULL) == -1)
>>           goto cleanup;
>> @@ -1210,12 +1161,8 @@ openvzDomainUndefineFlags(virDomainPtr dom,
>>       virCheckFlags(0, -1);
>>         openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> +    if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid)))
>>           goto cleanup;
>> -    }
> 
> Not related to your goal, but I wonder why the pattern here is
> different? Why does the driver need to be locked during the entire
> undefine operation?
> 

My assumption, old driver, no one updated it once the domainobjlist code
became self locking. I will add it to a future adjustment when I have to
mess with the Add code

Tks -

John

>>         if (openvzGetVEStatus(vm, &status, NULL) == -1)
>>           goto cleanup;
>> @@ -1255,15 +1202,8 @@ openvzDomainSetAutostart(virDomainPtr dom, int
>> autostart)
>>                              "--save", NULL };
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         openvzSetProgramSentinal(prog, vm->def->name);
>>       if (virRun(prog, NULL) < 0)
>> @@ -1284,15 +1224,8 @@ openvzDomainGetAutostart(virDomainPtr dom, int
>> *autostart)
>>       char *value = NULL;
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (openvzReadVPSConfigParam(strtoI(vm->def->name), "ONBOOT",
>> &value) < 0) {
>>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> @@ -1383,15 +1316,8 @@ static int
>> openvzDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
>>           return -1;
>>       }
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (nvcpus <= 0) {
>>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> @@ -1987,17 +1913,8 @@ openvzDomainInterfaceStats(virDomainPtr dom,
>>       virDomainNetDefPtr net = NULL;
>>       int ret = -1;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        char uuidstr[VIR_UUID_STRING_BUFLEN];
>> -        virUUIDFormat(dom->uuid, uuidstr);
>> -        virReportError(VIR_ERR_NO_DOMAIN,
>> -                       _("no domain with matching uuid '%s'"), uuidstr);
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>>         if (!virDomainObjIsActive(vm)) {
>>           virReportError(VIR_ERR_OPERATION_INVALID,
>> @@ -2082,13 +1999,8 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom,
>> const char *xml,
>>                     VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
>>         openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> +    if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid)))
>>           goto cleanup;
>> -    }
> 
> Same here. It's odd that the driver needs to be locked for the whole
> operation. But it's orthogonal to your work.
> 
> Reviewed-by: Jim Fehlig <jfehlig@xxxxxxxx>
> 
> Regards,
> Jim
> 
>>         if (virStrToLong_i(vm->def->name, NULL, 10, &veid) < 0) {
>>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> @@ -2230,15 +2142,8 @@ openvzDomainMigrateBegin3Params(virDomainPtr
>> domain,
>>       if (virTypedParamsValidate(params, nparams,
>> OPENVZ_MIGRATION_PARAMETERS) < 0)
>>           return NULL;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> -        goto cleanup;
>> -    }
>> +    if (!(vm = openvzDomObjFromDomain(driver, domain->uuid)))
>> +        return NULL;
>>         if (!virDomainObjIsActive(vm)) {
>>           virReportError(VIR_ERR_OPERATION_INVALID,
>> @@ -2394,15 +2299,8 @@ openvzDomainMigratePerform3Params(virDomainPtr
>> domain,
>>                                   &uri_str) < 0)
>>           goto cleanup;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> +    if (!(vm = openvzDomObjFromDomain(driver, domain->uuid)))
>>           goto cleanup;
>> -    }
>>         /* parse dst host:port from uri */
>>       uri = virURIParse(uri_str);
>> @@ -2504,15 +2402,8 @@ openvzDomainMigrateConfirm3Params(virDomainPtr
>> domain,
>>       if (virTypedParamsValidate(params, nparams,
>> OPENVZ_MIGRATION_PARAMETERS) < 0)
>>           goto cleanup;
>>   -    openvzDriverLock(driver);
>> -    vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
>> -    openvzDriverUnlock(driver);
>> -
>> -    if (!vm) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, "%s",
>> -                       _("no domain with matching uuid"));
>> +    if (!(vm = openvzDomObjFromDomain(driver, domain->uuid)))
>>           goto cleanup;
>> -    }
>>         if (cancelled) {
>>           if (openvzGetVEStatus(vm, &status, NULL) == -1)
>> @@ -2552,16 +2443,11 @@ openvzDomainHasManagedSaveImage(virDomainPtr
>> dom, unsigned int flags)
>>         virCheckFlags(0, -1);
>>   -    openvzDriverLock(driver);
>> -    obj = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>> -    openvzDriverUnlock(driver);
>> -    if (!obj) {
>> -        virReportError(VIR_ERR_NO_DOMAIN, NULL);
>> -        goto cleanup;
>> -    }
>> +    if (!(obj = openvzDomObjFromDomain(driver, dom->uuid)))
>> +        return -1;
>> +
>>       ret = 0;
>>   - cleanup:
>>       if (obj)
>>           virObjectUnlock(obj);
>>       return ret;
>>
> 

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux