9:26 a.m.
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(a)redhat.com>
> ---
> 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(a)suse.com>
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;
>