[libvirt] [PATCH v2 0/5] uml: Clean/fix test driver usage of FindBy{UUID|ID}
Patches 9-11 of larger series: v1: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html Changes since v1: * Patches 1 and 2 new - essentially they fix issues with how virDomainObjListRemove was used inconsistently. * Patch 3 should be the same or very similar to the former patch 9: https://www.redhat.com/archives/libvir-list/2018-March/msg00498.html * Patch 4 should be the same as former patch 10: https://www.redhat.com/archives/libvir-list/2018-March/msg00499.html * Patch 5 is altered to also fix virDomainObjListRemove inconsistencies John Ferlan (5): uml: Fix umlProcessAutoDestroyDom dom processing uml: Fix umlInotifyEvent dom object handling uml: Create accessors to virDomainObjListFindByUUID uml: Add more specific error message on failed FindBy call uml: Use virDomainObjListFindBy{UUID|ID}Ref src/uml/uml_driver.c | 353 +++++++++++++++++---------------------------------- 1 file changed, 116 insertions(+), 237 deletions(-) -- 2.13.6
There's no need to check if @dom exists before trying to call virDomainObjListRemove since it must exist due to prior checks. Additionally, if we do remove the @dom, then set it to NULL so that the virObjectUnlock isn't referencing something that is deleted. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ab7fa7f273..ff2e7ac66b 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -747,8 +747,10 @@ static int umlProcessAutoDestroyDom(void *payload, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (dom && !dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(data->driver->domains, dom); + dom = NULL; + } if (dom) virObjectUnlock(dom); -- 2.13.6
On Mon, Apr 02, 2018 at 03:06 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
There's no need to check if @dom exists before trying to call virDomainObjListRemove since it must exist due to prior checks.
Additionally, if we do remove the @dom, then set it to NULL so that the virObjectUnlock isn't referencing something that is deleted.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ab7fa7f273..ff2e7ac66b 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -747,8 +747,10 @@ static int umlProcessAutoDestroyDom(void *payload, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
- if (dom && !dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(data->driver->domains, dom); + dom = NULL; + }
if (dom) virObjectUnlock(dom); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Apr 02, 2018 at 09:06:12AM -0400, John Ferlan wrote:
There's no need to check if @dom exists before trying to call virDomainObjListRemove since it must exist due to prior checks.
Additionally, if we do remove the @dom, then set it to NULL so that the virObjectUnlock isn't referencing something that is deleted.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The virDomainObjListFindByName will return a locked and reffed object. If we call virDomainObjListRemove that will unlock the object upon return, thus we need to relock the object before making the call to virDomainObjEndAPI. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ff2e7ac66b..b84585d728 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -346,8 +346,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (e.mask & (IN_CREATE | IN_MODIFY)) { VIR_DEBUG("Got inotify domain startup '%s'", name); if (virDomainObjIsActive(dom)) { @@ -377,8 +379,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (umlIdentifyChrPTY(driver, dom) < 0) { VIR_WARN("Could not identify character devices for new domain"); umlShutdownVMDaemon(driver, dom, @@ -387,8 +391,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } } virDomainObjEndAPI(&dom); -- 2.13.6
On Mon, Apr 02, 2018 at 09:06:13AM -0400, John Ferlan wrote:
The virDomainObjListFindByName will return a locked and reffed object. If we call virDomainObjListRemove that will unlock the object upon return, thus we need to relock the object before making the call to virDomainObjEndAPI.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Matches what we do everywhere else, but ewwww if the caller owns the lock, and the 'dom' hasn't been freed entirely, it is pretty evil semantics to have the lock be lost. Why can't virDomainObjListRemove leave the lock state unchanged. It already unlocks, then locks, then unlocks again. A final lock would avoid such suprising semantics
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ff2e7ac66b..b84585d728 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -346,8 +346,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (e.mask & (IN_CREATE | IN_MODIFY)) { VIR_DEBUG("Got inotify domain startup '%s'", name); if (virDomainObjIsActive(dom)) { @@ -377,8 +379,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (umlIdentifyChrPTY(driver, dom) < 0) { VIR_WARN("Could not identify character devices for new domain"); umlShutdownVMDaemon(driver, dom, @@ -387,8 +391,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } } virDomainObjEndAPI(&dom); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 04/19/2018 09:51 AM, Daniel P. Berrangé wrote:
On Mon, Apr 02, 2018 at 09:06:13AM -0400, John Ferlan wrote:
The virDomainObjListFindByName will return a locked and reffed object. If we call virDomainObjListRemove that will unlock the object upon return, thus we need to relock the object before making the call to virDomainObjEndAPI.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Matches what we do everywhere else, but ewwww if the caller owns the lock, and the 'dom' hasn't been freed entirely, it is pretty evil semantics to have the lock be lost. Why can't virDomainObjListRemove leave the lock state unchanged. It already unlocks, then locks, then unlocks again. A final lock would avoid such suprising semantics
Yep... 100% agree. I'm trying to get to that type of model, but taking "baby steps" through each of the domain object consumers. Still on list are the vmware and vz to at least change the FindByUUID semantics. Once that happens changing domain object Add and Remove are the next step. I also have a series on the list undoing the lock fire dance (Erik's term ;-)) for secret, interface, nodedev, and storage. I was hoping to follow the same model for domain Remove. Thanks for the review, John
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ff2e7ac66b..b84585d728 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -346,8 +346,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (e.mask & (IN_CREATE | IN_MODIFY)) { VIR_DEBUG("Got inotify domain startup '%s'", name); if (virDomainObjIsActive(dom)) { @@ -377,8 +379,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } else if (umlIdentifyChrPTY(driver, dom) < 0) { VIR_WARN("Could not identify character devices for new domain"); umlShutdownVMDaemon(driver, dom, @@ -387,8 +391,10 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); + } } } virDomainObjEndAPI(&dom); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel
Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID. This will also generate a common error message including the failed uuidstr for lookup rather than just returning nothing in some instances. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 244 +++++++++++++++------------------------------------ 1 file changed, 70 insertions(+), 174 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b84585d728..ad526a1e6b 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -163,6 +163,39 @@ umlVMDriverUnlock(void) umlDriverUnlock(uml_driver); } + +static virDomainObjPtr +umlDomObjFromDomainLocked(struct uml_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 +umlDomObjFromDomain(struct uml_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + umlDriverLock(driver); + vm = umlDomObjFromDomainLocked(driver, uuid); + umlDriverUnlock(driver); + return vm; +} + + static virNWFilterCallbackDriver umlCallbackDriver = { .name = "UML", .vmFilterRebuild = umlVMFilterRebuild, @@ -1376,20 +1409,14 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, } static virDomainPtr umlDomainLookupByUUID(virConnectPtr conn, - const unsigned char *uuid) + const unsigned char *uuid) { struct uml_driver *driver = (struct uml_driver *)conn->privateData; virDomainObjPtr vm; virDomainPtr dom = NULL; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, uuid))) + return NULL; if (virDomainLookupByUUIDEnsureACL(conn, vm->def) < 0) goto cleanup; @@ -1435,13 +1462,8 @@ static int umlDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsActiveEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1461,13 +1483,8 @@ static int umlDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsPersistentEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1486,13 +1503,8 @@ static int umlDomainIsUpdated(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsUpdatedEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1645,14 +1657,8 @@ static int umlDomainShutdownFlags(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching id %d"), dom->id); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -1691,12 +1697,8 @@ umlDomainDestroyFlags(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching id %d"), dom->id); - goto cleanup; - } + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) + return -1; if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1734,14 +1736,8 @@ static char *umlDomainGetOSType(virDomainPtr dom) { virDomainObjPtr vm; char *type = NULL; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return NULL; if (virDomainGetOSTypeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1763,18 +1759,8 @@ umlDomainGetMaxMemory(virDomainPtr dom) virDomainObjPtr vm; unsigned long long ret = 0; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(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 = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1793,18 +1779,8 @@ static int umlDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(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 = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainSetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1830,18 +1806,8 @@ static int umlDomainSetMemory(virDomainPtr dom, unsigned long newmem) virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(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 = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainSetMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1874,15 +1840,8 @@ static int umlDomainGetInfo(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1923,15 +1882,8 @@ umlDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetStateEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1956,14 +1908,8 @@ static char *umlDomainGetXMLDesc(virDomainPtr dom, /* Flags checked by virDomainDefFormat */ umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -2023,13 +1969,8 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2123,12 +2064,8 @@ static int umlDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2221,14 +2158,8 @@ static int umlDomainAttachDevice(virDomainPtr dom, const char *xml) umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainAttachDeviceEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2341,14 +2272,8 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) int ret = -1; umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainDetachDeviceEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2411,13 +2336,8 @@ static int umlDomainGetAutostart(virDomainPtr dom, int ret = -1; umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainGetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2441,13 +2361,8 @@ static int umlDomainSetAutostart(virDomainPtr dom, int ret = -1; umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainSetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2517,15 +2432,8 @@ umlDomainBlockPeek(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainBlockPeekEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2581,7 +2489,6 @@ umlDomainOpenConsole(virDomainPtr dom, { struct uml_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; int ret = -1; virDomainChrDefPtr chr = NULL; size_t i; @@ -2589,13 +2496,8 @@ umlDomainOpenConsole(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - virUUIDFormat(dom->uuid, uuidstr); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2932,14 +2834,8 @@ umlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainHasManagedSaveImageEnsureACL(dom->conn, vm->def) < 0) goto cleanup; -- 2.13.6
On Mon, Apr 02, 2018 at 09:06:14AM -0400, John Ferlan wrote:
Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID. This will also generate a common error message including the failed uuidstr for lookup rather than just returning nothing in some instances.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 244 +++++++++++++++------------------------------------ 1 file changed, 70 insertions(+), 174 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Rather than an empty failed to find, let's provide a bit more knowledge about what we failed to find by using the name string or the id value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ad526a1e6b..b81da7cb2e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1393,7 +1393,8 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, umlDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching id '%d'"), id); goto cleanup; } @@ -1441,7 +1442,8 @@ static virDomainPtr umlDomainLookupByName(virConnectPtr conn, umlDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), name); goto cleanup; } -- 2.13.6
On Mon, Apr 02, 2018 at 09:06:15AM -0400, John Ferlan wrote:
Rather than an empty failed to find, let's provide a bit more knowledge about what we failed to find by using the name string or the id value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
For umlDomObjFromDomainLocked and umlDomainLookupByID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock. This means for some consumers we need to relock the @dom after a virDomainObjListRemove, but before calling virDomainObjEndAPI. The LookupByName already returns the ref counted and locked object, so this will make things more consistent. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 91 ++++++++++++++++++---------------------------------- 1 file changed, 32 insertions(+), 59 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b81da7cb2e..93d8a9f57c 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -171,7 +171,7 @@ umlDomObjFromDomainLocked(struct uml_driver *driver, virDomainObjPtr vm; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(vm = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -773,8 +773,7 @@ static int umlProcessAutoDestroyDom(void *payload, return 0; } - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, - uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(data->driver->domains, uuid))) { VIR_DEBUG("No domain object to kill"); return 0; } @@ -788,11 +787,10 @@ static int umlProcessAutoDestroyDom(void *payload, if (!dom->persistent) { virDomainObjListRemove(data->driver->domains, dom); - dom = NULL; + virObjectLock(dom); } - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); if (event) umlDomainEventQueue(data->driver, event); virHashRemoveEntry(data->driver->autodestroy, uuidstr); @@ -1389,7 +1387,7 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, virDomainPtr dom = NULL; umlDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); umlDriverUnlock(driver); if (!vm) { @@ -1404,8 +1402,7 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -1425,8 +1422,7 @@ static virDomainPtr umlDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -1473,8 +1469,7 @@ static int umlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1494,8 +1489,7 @@ static int umlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1514,8 +1508,7 @@ static int umlDomainIsUpdated(virDomainPtr dom) ret = obj->updated; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1676,8 +1669,7 @@ static int umlDomainShutdownFlags(virDomainPtr dom, cleanup: VIR_FREE(info); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1711,15 +1703,13 @@ umlDomainDestroyFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); if (!vm->persistent) { - virDomainObjListRemove(driver->domains, - vm); - vm = NULL; + virDomainObjListRemove(driver->domains, vm); + virObjectLock(vm); } ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -1748,8 +1738,7 @@ static char *umlDomainGetOSType(virDomainPtr dom) { goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return type; } @@ -1770,8 +1759,7 @@ umlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryTotal(vm->def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1797,8 +1785,7 @@ static int umlDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1830,8 +1817,7 @@ static int umlDomainSetMemory(virDomainPtr dom, unsigned long newmem) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1866,8 +1852,7 @@ static int umlDomainGetInfo(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1894,8 +1879,7 @@ umlDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1921,8 +1905,7 @@ static char *umlDomainGetXMLDesc(virDomainPtr dom, virDomainDefFormatConvertXMLFlags(flags)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1986,8 +1969,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_EVENT_STARTED_BOOTED); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -2034,8 +2016,7 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virDomainSaveConfig(driver->configDir, driver->caps, vm->newDef ? vm->newDef : vm->def) < 0) { - virDomainObjListRemove(driver->domains, - vm); + virDomainObjListRemove(driver->domains, vm); vm = NULL; goto cleanup; } @@ -2085,14 +2066,13 @@ static int umlDomainUndefineFlags(virDomainPtr dom, vm->persistent = 0; } else { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2198,8 +2178,7 @@ static int umlDomainAttachDevice(virDomainPtr dom, const char *xml) cleanup: virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2306,8 +2285,7 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) cleanup: virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2348,8 +2326,7 @@ static int umlDomainGetAutostart(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2413,8 +2390,7 @@ static int umlDomainSetAutostart(virDomainPtr dom, cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2477,8 +2453,7 @@ umlDomainBlockPeek(virDomainPtr dom, cleanup: VIR_FORCE_CLOSE(fd); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2545,8 +2520,7 @@ umlDomainOpenConsole(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2845,8 +2819,7 @@ umlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } -- 2.13.6
On Mon, Apr 02, 2018 at 09:06:16AM -0400, John Ferlan wrote:
For umlDomObjFromDomainLocked and umlDomainLookupByID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock. This means for some consumers we need to relock the @dom after a virDomainObjListRemove, but before calling virDomainObjEndAPI.
The LookupByName already returns the ref counted and locked object, so this will make things more consistent.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 91 ++++++++++++++++++---------------------------------- 1 file changed, 32 insertions(+), 59 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
ping? Beyond patch 1 which was reviewed. These are not uml specific, but rather more how domain objects are handled Tks - John On 04/02/2018 09:06 AM, John Ferlan wrote:
Patches 9-11 of larger series: v1: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
Changes since v1:
* Patches 1 and 2 new - essentially they fix issues with how virDomainObjListRemove was used inconsistently.
* Patch 3 should be the same or very similar to the former patch 9: https://www.redhat.com/archives/libvir-list/2018-March/msg00498.html
* Patch 4 should be the same as former patch 10: https://www.redhat.com/archives/libvir-list/2018-March/msg00499.html
* Patch 5 is altered to also fix virDomainObjListRemove inconsistencies
John Ferlan (5): uml: Fix umlProcessAutoDestroyDom dom processing uml: Fix umlInotifyEvent dom object handling uml: Create accessors to virDomainObjListFindByUUID uml: Add more specific error message on failed FindBy call uml: Use virDomainObjListFindBy{UUID|ID}Ref
src/uml/uml_driver.c | 353 +++++++++++++++++---------------------------------- 1 file changed, 116 insertions(+), 237 deletions(-)
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer