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
