[libvirt] [PATCH 0/3] libxl: finish use of {Begin, End}API pattern in APIs
This is a follow-up to John and I's recent work to improve locking and ref counting of virDomainObj in the libxl driver. These patches change the remaining APIs to use the begin and end API pattern used throughout libxl and other drivers. Jim Fehlig (3): libxl: lock virDomainObj after ListRemove libxl: call EndJob in error case libxl: convert DefineXMLFlags to use begin/end API pattern src/libxl/libxl_driver.c | 29 ++++++++++++++++------------- src/libxl/libxl_migration.c | 7 +++++-- 2 files changed, 21 insertions(+), 15 deletions(-) -- 2.16.2
Most libxl driver API use the pattern of lock and add a ref to virDomainObj, perform API, then decrement ref and unlock in virDomainEndAPI. In some cases the API may call virDomainObjListRemove, which unlocks the virDomainObj. Relock the object in such cases so EndAPI is called with a locked object. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 22 +++++++++++++--------- src/libxl/libxl_migration.c | 7 +++++-- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 89432d03c..ac61fb322 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1056,7 +1056,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } goto cleanup; } @@ -1065,7 +1065,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, (flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) { if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); goto cleanup; } goto endjob; @@ -1417,8 +1417,10 @@ libxlDomainDestroyFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); libxlDomainCleanup(driver, vm); - if (!vm->persistent) + if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); + virObjectLock(vm); + } ret = 0; @@ -1822,7 +1824,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, cleanup: if (remove_dom && vm) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } virDomainObjEndAPI(&vm); return ret; @@ -1877,7 +1879,7 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } goto cleanup; } @@ -1885,8 +1887,10 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, ret = libxlDomainStartRestore(driver, vm, (flags & VIR_DOMAIN_SAVE_PAUSED) != 0, fd, hdr.version); - if (ret < 0 && !vm->persistent) + if (ret < 0 && !vm->persistent) { virDomainObjListRemove(driver->domains, vm); + virObjectLock(vm); + } libxlDomainObjEndJob(driver, vm); @@ -1995,7 +1999,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) cleanup: if (remove_dom && vm) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } virDomainObjEndAPI(&vm); if (event) @@ -2056,7 +2060,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) cleanup: if (remove_dom && vm) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } virDomainObjEndAPI(&vm); VIR_FREE(name); @@ -2880,7 +2884,7 @@ libxlDomainUndefineFlags(virDomainPtr dom, vm->persistent = 0; } else { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } ret = 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7dc39ae02..324a1adde 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -299,7 +299,7 @@ libxlDoMigrateReceive(void *opaque) cleanup: if (remove_dom) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } virDomainObjEndAPI(&vm); } @@ -1336,8 +1336,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn, VIR_DOMAIN_SHUTOFF_FAILED); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!vm->persistent) + if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); + /* Caller passed a locked vm and expects the same on return */ + virObjectLock(vm); + } } if (event) -- 2.16.2
If starting the domain fails in libxlDomainCreateXML, we mistakenly jumped to cleanup without calling libxlDomainObjEndJob. Remove the jump to 'cleanup'. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index ac61fb322..3ebeacca8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1066,7 +1066,6 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); virObjectLock(vm); - goto cleanup; } goto endjob; } -- 2.16.2
Similar to other uses of virDomainObjListAdd, on success add a ref to the virDomainObj so that virDomainObjEndAPI can be called as usual. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3ebeacca8..86cfa0d9a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2794,6 +2794,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag &oldDef))) goto cleanup; + virObjectRef(vm); def = NULL; vm->persistent = 1; @@ -2801,7 +2802,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag cfg->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); goto cleanup; } @@ -2815,8 +2816,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag cleanup: virDomainDefFree(def); virDomainDefFree(oldDef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); -- 2.16.2
On 03/19/2018 07:14 PM, Jim Fehlig wrote:
This is a follow-up to John and I's recent work to improve locking and ref counting of virDomainObj in the libxl driver. These patches change the remaining APIs to use the begin and end API pattern used throughout libxl and other drivers.
Jim Fehlig (3): libxl: lock virDomainObj after ListRemove libxl: call EndJob in error case libxl: convert DefineXMLFlags to use begin/end API pattern
src/libxl/libxl_driver.c | 29 ++++++++++++++++------------- src/libxl/libxl_migration.c | 7 +++++-- 2 files changed, 21 insertions(+), 15 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> (series) John
participants (2)
-
Jim Fehlig -
John Ferlan