[libvirt] [PATCH 0/6] Complete cleanup of domain object usage
This should be the "last time" (famous last words) needing to alter this processing. I hope to get more than one pair of eyes looking at the series. I've looked at it long enough to feel comfortable with the changes, but I also know I probably have looked at it so long that I could have missed something! The first 3 patches set up the necessary infrastructure. Patches 4 & 5 are just something I found in libxl while doing this - they could be extracted and considered separately if need be; however, since I was thinking about @vm referencing - it just seemed to fit. Since I altered @vm it only seemed right to do the same for @conn. While it could be said that neither condition could happen - I figured better safe than sorry and I think it follows how this type of logic has been handled elsewhere in qemu. Patch 6 is where all the magic happens. I tried a few different ways to rework and split the Add/Remove processing, but either way I was left with rather ugly patches that required touching the same places, so I capitulated and combined them since there really is a delicate balance between those paths that have added a virObjectRef after the virDomainObjListAdd and those that just use virObjectUnlock on the returned ListAdd objects. Converting the latter to use Ref caused more "busy work" and equally large patches. The busy work was mosting around the processing required during ListAdd if something happened which resulted in ListRemove needing to be called. The "general change" is that instead of having ListAdd return an object with 2 refs and 1 lock it will now return an object with 3 refs and 1 lock. With that returned object, some drivers would perform an ObjectRef while others did not. For those that did not, they would use ObjectUnlock when done with the object so that when leaving whatever Create routine there was there would be at least 2 Refs on the object. The change here is to have them all be consistent. What follows is a "general description" of what was done for each. For bhyve, openvz, test, uml, and vmware (e.g. domain drivers that do not use virObjectRef on the returned virDomainObjListAdd object): Create/Add processing: -> Use EndAPI instead of ObjectUnlock -> Remove setting vm = NULL if the ListRemove was called to allow EndAPI processing to perform last Unref and Unlock NB: ListRemove would remove 2 refs and unlock, so it "worked", but would "consume" the returned object Undefine/Destroy processing: -> Alter ListRemove and ObjectLock to just be ListRemove allowing the EndAPI processing from a FindBy* to perform last Unref and Unlock For libxl, lxc, qemu, and vz (e.g. domain drivers that use virObjectRef on the returned virDomainObjListAdd object): Create/Add processing: -> Remove the ObjectRef -> Remove the ObjectLock when needing to call ListRemove to allow EndAPI processing to perform the last Unref and Unlock NB: ListRemove no longer Unlock's the returned object Undefine/Destroy processing: -> Alter ListRemove and ObjectLock to just be ListRemove allowing the EndAPI processing from a FindBy* to perform last Unref and Unlock NB: For the AutoDestroy type logic, no change is required since in the long run the object was left with the correct number of refs after create processing ran. The issue is more the direct mgmt of object during Add/Remove rather than mgmt of object once defined. John Ferlan (6): conf: Split FindBy{UUID|Name} into locked helpers conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd conf: Move and use virDomainObjListRemoveLocked libxl: Add refcnt for args->vm during migration libxl: Add refcnt for args->conn during migration conf: Clean up object referencing for Add and Remove src/bhyve/bhyve_driver.c | 21 ++---- src/conf/virdomainobjlist.c | 165 ++++++++++++++++++++++++++++---------------- src/libxl/libxl_driver.c | 64 ++++------------- src/libxl/libxl_migration.c | 41 ++++------- src/lxc/lxc_driver.c | 21 ++---- src/lxc/lxc_process.c | 17 +++-- src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 20 ++---- src/qemu/qemu_domain.c | 17 +---- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_migration.c | 3 +- src/test/test_driver.c | 56 +++++---------- src/uml/uml_driver.c | 37 +++------- src/vmware/vmware_conf.c | 3 +- src/vmware/vmware_driver.c | 17 ++--- src/vz/vz_driver.c | 1 - src/vz/vz_sdk.c | 3 - 17 files changed, 192 insertions(+), 302 deletions(-) -- 2.13.6
Create helpers virDomainObjListFindByUUIDLocked and virDomainObjListFindByNameLocked to avoid the need to lock the domain object list leaving that task for the caller. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d57ed10a5f..9aa2abd8c3 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -133,19 +133,16 @@ virDomainObjListFindByID(virDomainObjListPtr doms, } -virDomainObjPtr -virDomainObjListFindByUUID(virDomainObjListPtr doms, - const unsigned char *uuid) +static virDomainObjPtr +virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms, + const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj; - virObjectRWLockRead(doms); virUUIDFormat(uuid, uuidstr); - obj = virHashLookup(doms->objs, uuidstr); virObjectRef(obj); - virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { @@ -158,15 +155,36 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, } -virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, - const char *name) +/** + * @doms: Locked domain object list + * @uuid: UUID to search the doms->objs table + * + * Lookup the @uuid in the doms->objs hash table and return a + * locked and ref counted domain object if found. Caller is + * expected to use the virDomainObjEndAPI when done with the object. + */ +virDomainObjPtr +virDomainObjListFindByUUID(virDomainObjListPtr doms, + const unsigned char *uuid) { virDomainObjPtr obj; virObjectRWLockRead(doms); + obj = virDomainObjListFindByUUIDLocked(doms, uuid); + virObjectRWUnlock(doms); + + return obj; +} + + +static virDomainObjPtr +virDomainObjListFindByNameLocked(virDomainObjListPtr doms, + const char *name) +{ + virDomainObjPtr obj; + obj = virHashLookup(doms->objsName, name); virObjectRef(obj); - virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { @@ -180,6 +198,28 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, /** + * @doms: Locked domain object list + * @name: Name to search the doms->objsName table + * + * Lookup the @name in the doms->objsName hash table and return a + * locked and ref counted domain object if found. Caller is expected + * to use the virDomainObjEndAPI when done with the object. + */ +virDomainObjPtr +virDomainObjListFindByName(virDomainObjListPtr doms, + const char *name) +{ + virDomainObjPtr obj; + + virObjectRWLockRead(doms); + obj = virDomainObjListFindByNameLocked(doms, name); + virObjectRWUnlock(doms); + + return obj; +} + + +/** * @doms: Domain object list pointer * @vm: Domain object to be added * -- 2.13.6
On Tue, Apr 24, 2018 at 08:28:04AM -0400, John Ferlan wrote:
Create helpers virDomainObjListFindByUUIDLocked and virDomainObjListFindByNameLocked to avoid the need to lock the domain object list leaving that task for the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d57ed10a5f..9aa2abd8c3 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -133,19 +133,16 @@ virDomainObjListFindByID(virDomainObjListPtr doms, }
-virDomainObjPtr -virDomainObjListFindByUUID(virDomainObjListPtr doms, - const unsigned char *uuid) +static virDomainObjPtr +virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms, + const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj;
- virObjectRWLockRead(doms); virUUIDFormat(uuid, uuidstr); - obj = virHashLookup(doms->objs, uuidstr); virObjectRef(obj); - virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { @@ -158,15 +155,36 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, }
-virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, - const char *name) +/** + * @doms: Locked domain object list
^It's not, you actually lock it here...I guess you sort of wanted to attach the commentary to the *Locked version of the function but then changed your mind and combined your thoughts from both? :)...same for the other function. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 05/03/2018 06:38 AM, Erik Skultety wrote:
On Tue, Apr 24, 2018 at 08:28:04AM -0400, John Ferlan wrote:
Create helpers virDomainObjListFindByUUIDLocked and virDomainObjListFindByNameLocked to avoid the need to lock the domain object list leaving that task for the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d57ed10a5f..9aa2abd8c3 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -133,19 +133,16 @@ virDomainObjListFindByID(virDomainObjListPtr doms, }
-virDomainObjPtr -virDomainObjListFindByUUID(virDomainObjListPtr doms, - const unsigned char *uuid) +static virDomainObjPtr +virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms, + const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj;
- virObjectRWLockRead(doms); virUUIDFormat(uuid, uuidstr); - obj = virHashLookup(doms->objs, uuidstr); virObjectRef(obj); - virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { @@ -158,15 +155,36 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, }
-virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, - const char *name) +/** + * @doms: Locked domain object list
^It's not, you actually lock it here...I guess you sort of wanted to attach the commentary to the *Locked version of the function but then changed your mind and combined your thoughts from both? :)...same for the other function.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
oh, right - exactly what I did - the comment was above the Locked functions, but then I moved it. I changed both : s/Locked d/D/ Tks - John
Use the FindBy{UUID|Name}Locked helpers which will return a locked and ref counted object rather than the direct virHashLookup and virObjectLock of the returned object. We'll need to temporarily virObjectUnref when we assign a new domain @def, but that will change shortly when virDomainObjListAddObjLocked returns the correct reference counted object. Use the virDomainObjEndAPI in the error path to Unref/Unlock for the corresponding Unref/Unlock of either the FindBy* return or the virDomainObjNew since both return a reffed/locked object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 9aa2abd8c3..6752f6c572 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, if (oldDef) *oldDef = NULL; - virUUIDFormat(def->uuid, uuidstr); - /* See if a VM with matching UUID already exists */ - if ((vm = virHashLookup(doms->objs, uuidstr))) { - virObjectLock(vm); + if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) { /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(vm->def->name, def->name)) { virUUIDFormat(vm->def->uuid, uuidstr); @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), oldDef); + /* XXX: Temporary until this API is fixed to return a locked and + * refcnt'd object */ + virObjectUnref(vm); } else { /* UUID does not match, but if a name matches, refuse it */ - if ((vm = virHashLookup(doms->objsName, def->name))) { - virObjectLock(vm); + if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) { virUUIDFormat(vm->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("domain '%s' already exists with uuid %s"), @@ -329,18 +328,15 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, goto cleanup; vm->def = def; - if (virDomainObjListAddObjLocked(doms, vm) < 0) { - virDomainObjEndAPI(&vm); - return NULL; - } + if (virDomainObjListAddObjLocked(doms, vm) < 0) + goto error; } cleanup: return vm; error: - virObjectUnlock(vm); - vm = NULL; - goto cleanup; + virDomainObjEndAPI(&vm); + return NULL; } -- 2.13.6
On Tue, Apr 24, 2018 at 08:28:05AM -0400, John Ferlan wrote:
Use the FindBy{UUID|Name}Locked helpers which will return a locked and ref counted object rather than the direct virHashLookup and virObjectLock of the returned object. We'll need to temporarily virObjectUnref when we assign a new domain @def, but that will change shortly when virDomainObjListAddObjLocked returns the correct reference counted object.
Use the virDomainObjEndAPI in the error path to Unref/Unlock for the corresponding Unref/Unlock of either the FindBy* return or the virDomainObjNew since both return a reffed/locked object.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 9aa2abd8c3..6752f6c572 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, if (oldDef) *oldDef = NULL;
- virUUIDFormat(def->uuid, uuidstr); - /* See if a VM with matching UUID already exists */ - if ((vm = virHashLookup(doms->objs, uuidstr))) { - virObjectLock(vm); + if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) { /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(vm->def->name, def->name)) { virUUIDFormat(vm->def->uuid, uuidstr); @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), oldDef); + /* XXX: Temporary until this API is fixed to return a locked and + * refcnt'd object */ + virObjectUnref(vm);
I didn't bother trying, but would the tests pass even without ^this temporary unref? Because if they do, we should drop it, since you do that anyway within the same series, so who cares... With or without the adjustment Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 05/03/2018 12:02 PM, Erik Skultety wrote:
On Tue, Apr 24, 2018 at 08:28:05AM -0400, John Ferlan wrote:
Use the FindBy{UUID|Name}Locked helpers which will return a locked and ref counted object rather than the direct virHashLookup and virObjectLock of the returned object. We'll need to temporarily virObjectUnref when we assign a new domain @def, but that will change shortly when virDomainObjListAddObjLocked returns the correct reference counted object.
Use the virDomainObjEndAPI in the error path to Unref/Unlock for the corresponding Unref/Unlock of either the FindBy* return or the virDomainObjNew since both return a reffed/locked object.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 9aa2abd8c3..6752f6c572 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, if (oldDef) *oldDef = NULL;
- virUUIDFormat(def->uuid, uuidstr); - /* See if a VM with matching UUID already exists */ - if ((vm = virHashLookup(doms->objs, uuidstr))) { - virObjectLock(vm); + if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) { /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(vm->def->name, def->name)) { virUUIDFormat(vm->def->uuid, uuidstr); @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), oldDef); + /* XXX: Temporary until this API is fixed to return a locked and + * refcnt'd object */ + virObjectUnref(vm);
I didn't bother trying, but would the tests pass even without ^this temporary unref? Because if they do, we should drop it, since you do that anyway within the same series, so who cares...
With or without the adjustment Reviewed-by: Erik Skultety <eskultet@redhat.com>
Yes, the tests would pass. It's needed temporarily because virDomainObjListFindByUUIDLocked returns a reffed/locked @vm; whereas, virHashLookup just returns @vm which was locked before return. The caller expects to receive @vm without that extra ref. All things being equal removing this would temporarily leak @vm. I was trying to be "correct" through each stage of the changes. And yes by patch 6 (or 4 if things are rearrange) it's a moot point because the Add API will return the correct number of ref's on @vm. Tks - John
Rather than open code within virDomainObjListRemove, just call the *Locked function. Additionally, add comments to virDomainObjListRemove to describe the usage model. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 64 +++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 6752f6c572..5725040552 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -355,26 +355,50 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, } -/* - * The caller must hold a lock on the driver owning 'doms', - * and must also have locked 'dom', to ensure no one else - * is either waiting for 'dom' or still using it +/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove' + * requirements + * + * Can be used to remove current element while iterating with + * virDomainObjListForEach */ -void virDomainObjListRemove(virDomainObjListPtr doms, - virDomainObjPtr dom) +void +virDomainObjListRemoveLocked(virDomainObjListPtr doms, + virDomainObjPtr dom) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - dom->removing = true; virUUIDFormat(dom->def->uuid, uuidstr); - virObjectRef(dom); - virObjectUnlock(dom); - virObjectRWLockWrite(doms); - virObjectLock(dom); virHashRemoveEntry(doms->objs, uuidstr); virHashRemoveEntry(doms->objsName, dom->def->name); virObjectUnlock(dom); +} + + +/** + * @doms: Pointer to the domain object list + * @dom: Domain pointer from either after Add or FindBy* API where the + * @dom was successfully added to both the doms->objs and ->objsName + * hash tables that now would need to be removed. + * + * The caller must hold a lock on the driver owning 'doms', + * and must also have locked and ref counted 'dom', to ensure + * no one else is either waiting for 'dom' or still using it. + * + * When this function returns, @dom will be removed from the hash + * tables, unlocked, and returned with the refcnt that was present + * upon entry. + */ +void +virDomainObjListRemove(virDomainObjListPtr doms, + virDomainObjPtr dom) +{ + dom->removing = true; + virObjectRef(dom); + virObjectUnlock(dom); + virObjectRWLockWrite(doms); + virObjectLock(dom); + virDomainObjListRemoveLocked(doms, dom); virObjectUnref(dom); virObjectRWUnlock(doms); } @@ -446,24 +470,6 @@ virDomainObjListRename(virDomainObjListPtr doms, return ret; } -/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove' - * requirements - * - * Can be used to remove current element while iterating with - * virDomainObjListForEach - */ -void virDomainObjListRemoveLocked(virDomainObjListPtr doms, - virDomainObjPtr dom) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->def->uuid, uuidstr); - - virHashRemoveEntry(doms->objs, uuidstr); - virHashRemoveEntry(doms->objsName, dom->def->name); - virObjectUnlock(dom); -} - static virDomainObjPtr virDomainObjListLoadConfig(virDomainObjListPtr doms, -- 2.13.6
On Tue, Apr 24, 2018 at 08:28:06AM -0400, John Ferlan wrote:
Rather than open code within virDomainObjListRemove, just call the *Locked function.
Additionally, add comments to virDomainObjListRemove to describe the usage model.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
When adding the @vm to the @args for usage during a thread or callback, let's add the reference to it at the time of adding to ensure nothing else deletes it. The corresponding Unref is then added to the Dispose function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index d7b494b392..7fe352306c 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj) libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->vm); } static int @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, goto error; args->conn = dconn; - args->vm = vm; + args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; /* Receive from pipeOut */ @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, goto error; args->conn = dconn; - args->vm = vm; + args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks; args->nsocks = nsocks; -- 2.13.6
On Tue, Apr 24, 2018 at 02:28 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
When adding the @vm to the @args for usage during a thread or callback, let's add the reference to it at the time of adding to ensure nothing else deletes it. The corresponding Unref is then added to the Dispose function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index d7b494b392..7fe352306c 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->vm); }
static int @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, goto error;
args->conn = dconn; - args->vm = vm; + args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; /* Receive from pipeOut */ @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, goto error;
args->conn = dconn; - args->vm = vm; + args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks; args->nsocks = nsocks; -- 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
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7fe352306c..d37a4a687a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj) libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->conn); virObjectUnref(args->vm); } @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error; - args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error; - args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks; -- 2.13.6
On Tue, Apr 24, 2018 at 02:28 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7fe352306c..d37a4a687a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->conn); virObjectUnref(args->vm); }
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks; -- 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 Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7fe352306c..d37a4a687a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->conn); virObjectUnref(args->vm); }
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks;
Do you actually have a use case, where the conn object would be destroyed before the migration finishes in a way that this could cause a SIGSEGV? Erik
On 05/03/2018 08:34 AM, Erik Skultety wrote:
On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7fe352306c..d37a4a687a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->conn); virObjectUnref(args->vm); }
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks;
Do you actually have a use case, where the conn object would be destroyed before the migration finishes in a way that this could cause a SIGSEGV?
Nope - just fear and protection any such possibility. We're saving a pointer to some object in an object and possibly dereferencing it at some point in time in the future during libxlDoMigrateDstReceive. Other places where we do similar things w/ conn we also add a Ref to it. And yes, patch 4 and 5 could be separate, perhaps more so this patch than patch 4 because of how patch 6 changes @vm refcnt. It shouldn't be possible to have a failure condition for either Ref, but those are always "famous last words" and really difficult to diagnose after the fact. It doesn't hurt to Ref and then Unref AFAIK. John
On Thu, May 03, 2018 at 09:02:35AM -0400, John Ferlan wrote:
On 05/03/2018 08:34 AM, Erik Skultety wrote:
On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 7fe352306c..d37a4a687a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
libxlMigrationCookieFree(args->migcookie); VIR_FREE(args->socks); + virObjectUnref(args->conn); virObjectUnref(args->vm); }
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->migcookie = mig; @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, if (!(args = virObjectNew(libxlMigrationDstArgsClass))) goto error;
- args->conn = dconn; + args->conn = virObjectRef(dconn); args->vm = virObjectRef(vm); args->flags = flags; args->socks = socks;
Do you actually have a use case, where the conn object would be destroyed before the migration finishes in a way that this could cause a SIGSEGV?
Nope - just fear and protection any such possibility. We're saving a pointer to some object in an object and possibly dereferencing it at some point in time in the future during libxlDoMigrateDstReceive. Other places where we do similar things w/ conn we also add a Ref to it.
And yes, patch 4 and 5 could be separate, perhaps more so this patch than patch 4 because of how patch 6 changes @vm refcnt.
Doesn't have any effect on 4, whether you apply 4-5 before 6 or after, it's an independent fix, but in the end it doesn't really matter, since you've got my RB for both, so separating them now is irrelevant :).
It shouldn't be possible to have a failure condition for either Ref, but those are always "famous last words" and really difficult to diagnose after the fact. It doesn't hurt to Ref and then Unref AFAIK.
It doesn't, I was just curious, because I didn't see anything similar in qemu migration code, but I wasn't really thorough, so it could have been done prior to entering the migration code in qemu_driver.c, I'm not going to dig deeper, it's a simple straightforward Ref/Unref change. Reviewed-by: Erik Skultety <eskultet@redhat.com> PS: I still think you could move 4,5 after 6, since to me the causality of changes is more evident.
On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
Since the @dconn reference via args->conn will be used via a thread or callback, let's make sure memory associated with it isn't free'd unexpectedly before we use it. The Unref will be done when the object is Dispose'd.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Also, yeah, as you figured and noted in the cover letter, these two libxl patches would probably be better off in a separate series IMO. Erik
When adding a new object to the domain object list, there should have been 2 virObjectRef calls made one for each list into which the object was placed to match the 2 virObjectUnref calls that would occur during Remove as part of virHashRemoveEntry when virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew. Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency by calling virObjectRef upon successful return from virDomainObjListAdd in order to use virDomainObjEndAPI when done with the returned @vm. While others (bhyve, openvz, test, and vmware) handled this via only calling virObjectUnlock upon successful return from virDomainObjListAdd. This patch will "unify" the approach to use virDomainObjEndAPI for any @vm successfully returned from virDomainObjListAdd. Because list removal is so tightly coupled with list addition, this patch fixes the list removal algorithm to return the object as entered - "locked and reffed". This way, the callers can then decide how to uniformly handle add/remove success and failure. This removes the onus on the caller to "specially handle" the @vm during removal processing. The Add/Remove logic allows for some logic simplification such as in libxl where we can Remove the @vm directly rather than needing to set a @remove_dom boolean and removing after the libxlDomainObjEndJob completes as the @vm is locked/reffed. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/bhyve/bhyve_driver.c | 21 +++++---------- src/conf/virdomainobjlist.c | 33 +++++++++++------------ src/libxl/libxl_driver.c | 64 +++++++++------------------------------------ src/libxl/libxl_migration.c | 31 +++++----------------- src/lxc/lxc_driver.c | 21 ++++----------- src/lxc/lxc_process.c | 17 ++++++------ src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 20 +++++--------- src/qemu/qemu_domain.c | 17 +----------- src/qemu/qemu_driver.c | 6 +---- src/qemu/qemu_migration.c | 3 +-- src/test/test_driver.c | 56 ++++++++++++--------------------------- src/uml/uml_driver.c | 37 +++++++------------------- src/vmware/vmware_conf.c | 3 +-- src/vmware/vmware_driver.c | 17 ++++-------- src/vz/vz_driver.c | 1 - src/vz/vz_sdk.c | 3 --- 17 files changed, 99 insertions(+), 253 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 768578a43f..6001f0806c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -550,7 +550,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virDomainSaveConfig(BHYVE_CONFIG_DIR, caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(privconn->domains, vm); - vm = NULL; goto cleanup; } @@ -566,8 +565,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag virObjectUnref(caps); virDomainDefFree(def); virDomainDefFree(oldDef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); @@ -609,12 +607,10 @@ bhyveDomainUndefine(virDomainPtr domain) VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(privconn->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -958,10 +954,8 @@ bhyveDomainCreateXML(virConnectPtr conn, VIR_DOMAIN_RUNNING_BOOTED, start_flags) < 0) { /* If domain is not persistent, remove its data */ - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(privconn->domains, vm); - vm = NULL; - } goto cleanup; } @@ -974,8 +968,7 @@ bhyveDomainCreateXML(virConnectPtr conn, cleanup: virObjectUnref(caps); virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); @@ -1007,10 +1000,8 @@ bhyveDomainDestroy(virDomainPtr dom) VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(privconn->domains, vm); - virObjectLock(vm); - } cleanup: virDomainObjEndAPI(&vm); diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 5725040552..a30b3908f0 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -226,9 +226,13 @@ virDomainObjListFindByName(virDomainObjListPtr doms, * Upon entry @vm should have at least 1 ref and be locked. * * Add the @vm into the @doms->objs and @doms->objsName hash - * tables. + * tables. Once successfully added into a table, increase the + * reference count since upon removal in virHashRemoveEntry + * the virObjectUnref will be called since the hash tables were + * configured to call virObjectFreeHashData when the object is + * removed from the hash table. * - * Returns 0 on success with 2 references and locked + * Returns 0 on success with 3 references and locked * -1 on failure with 1 reference and locked */ static int @@ -240,15 +244,12 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms, virUUIDFormat(vm->def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) return -1; + virObjectRef(vm); if (virHashAddEntry(doms->objsName, vm->def->name, vm) < 0) { - virObjectRef(vm); virHashRemoveEntry(doms->objs, uuidstr); return -1; } - - /* Since domain is in two hash tables, increment the - * reference counter */ virObjectRef(vm); return 0; @@ -266,6 +267,9 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms, * the @def being added is assumed to represent a * live config, not a future inactive config * + * The returned @vm from this function will be locked and ref + * counted. The caller is expected to use virDomainObjEndAPI + * when it completes usage. */ static virDomainObjPtr virDomainObjListAddLocked(virDomainObjListPtr doms, @@ -311,9 +315,6 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), oldDef); - /* XXX: Temporary until this API is fixed to return a locked and - * refcnt'd object */ - virObjectUnref(vm); } else { /* UUID does not match, but if a name matches, refuse it */ if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) { @@ -371,7 +372,6 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, virHashRemoveEntry(doms->objs, uuidstr); virHashRemoveEntry(doms->objsName, dom->def->name); - virObjectUnlock(dom); } @@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, * no one else is either waiting for 'dom' or still using it. * * When this function returns, @dom will be removed from the hash - * tables, unlocked, and returned with the refcnt that was present - * upon entry. + * tables and returned with lock and refcnt that was present upon entry. */ void virDomainObjListRemove(virDomainObjListPtr doms, @@ -453,9 +452,11 @@ virDomainObjListRename(virDomainObjListPtr doms, if (virHashAddEntry(doms->objsName, new_name, dom) < 0) goto cleanup; - /* Okay, this is crazy. virHashAddEntry() does not increment - * the refcounter of @dom, but virHashRemoveEntry() does - * decrement it. We need to work around it. */ + /* Increment the refcnt for @new_name. We're about to remove + * the @old_name which will cause the refcnt to be decremented + * via the virObjectUnref call made during the virObjectFreeHashData + * as a result of removing something from the object list hash + * table as set up during virDomainObjListNew. */ virObjectRef(dom); rc = callback(dom, new_name, flags, opaque); @@ -624,7 +625,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, if (dom) { if (!liveStatus) dom->persistent = 1; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } else { VIR_ERROR(_("Failed to load config for domain '%s'"), entry->d_name); } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 55a93a489b..34adef8d48 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -450,13 +450,8 @@ libxlReconnectDomain(virDomainObjPtr vm, error: libxlDomainCleanup(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemoveLocked(driver->domains, vm); - - /* virDomainObjListRemoveLocked leaves the object unlocked, - * lock it again to factorize more code. */ - virObjectLock(vm); - } goto cleanup; } @@ -605,7 +600,6 @@ libxlAddDom0(libxlDriverPrivatePtr driver) 0, &oldDef))) goto cleanup; - def = NULL; vm->persistent = 1; @@ -626,8 +620,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) libxl_dominfo_dispose(&d_info); virDomainDefFree(def); virDomainDefFree(oldDef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1043,23 +1036,18 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } if (libxlDomainStartNew(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto endjob; } @@ -1396,10 +1384,8 @@ 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; @@ -1759,7 +1745,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - bool remove_dom = false; #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME virReportUnsupportedError(); @@ -1791,7 +1776,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; @@ -1799,10 +1784,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); return ret; } @@ -1850,24 +1831,19 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } 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); @@ -1893,7 +1869,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virObjectEventPtr event = NULL; - bool remove_dom = false; bool paused = false; int ret = -1; @@ -1951,7 +1926,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); } ret = 0; @@ -1972,10 +1947,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); @@ -1990,7 +1961,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm = NULL; char *name = NULL; int ret = -1; - bool remove_dom = false; virCheckFlags(0, -1); @@ -2023,7 +1993,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; @@ -2031,10 +2001,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); VIR_FREE(name); return ret; @@ -2765,16 +2731,14 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag 0, &oldDef))) goto cleanup; - - virObjectRef(vm); def = NULL; + vm->persistent = 1; if (virDomainSaveConfig(cfg->configDir, cfg->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); goto cleanup; } @@ -2851,12 +2815,10 @@ libxlDomainUndefineFlags(virDomainPtr dom, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index d37a4a687a..206b878292 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -265,7 +265,6 @@ libxlDoMigrateDstReceive(void *opaque) int recvfd = args->recvfd; size_t i; int ret; - bool remove_dom = 0; virObjectRef(vm); virObjectLock(vm); @@ -280,7 +279,7 @@ libxlDoMigrateDstReceive(void *opaque) args->migcookie->xenMigStreamVer); if (ret < 0 && !vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); /* Remove all listen socks from event handler, and close them. */ for (i = 0; i < nsocks; i++) { @@ -296,10 +295,6 @@ libxlDoMigrateDstReceive(void *opaque) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); } @@ -581,9 +576,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (taint_hook) { @@ -633,10 +627,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, VIR_FORCE_CLOSE(dataFD[0]); virObjectUnref(args); /* Remove virDomainObj from domain list */ - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } done: virDomainObjEndAPI(&vm); @@ -680,9 +672,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (taint_hook) { @@ -807,10 +798,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, priv->migrationPort = 0; } /* Remove virDomainObj from domain list */ - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } done: VIR_FREE(xmlout); @@ -1336,11 +1325,8 @@ libxlDomainMigrationDstFinish(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) @@ -1392,11 +1378,8 @@ libxlDomainMigrationSrcConfirm(libxlDriverPrivatePtr driver, if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); - if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) { + if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) virDomainObjListRemove(driver->domains, vm); - /* Caller passed a locked vm and expects the same on return */ - virObjectLock(vm); - } ret = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ca01d369d5..5fbc3fe666 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -478,14 +478,12 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) 0, &oldDef))) goto cleanup; - virObjectRef(vm); def = NULL; vm->persistent = 1; if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); goto cleanup; } @@ -546,12 +544,10 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -1233,14 +1229,11 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } @@ -1250,10 +1243,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, VIR_DOMAIN_RUNNING_BOOTED) < 0) { virDomainAuditStart(vm, "booted", false); virLXCDomainObjEndJob(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } @@ -1523,10 +1514,8 @@ lxcDomainDestroyFlags(virDomainPtr dom, endjob: virLXCDomainObjEndJob(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } cleanup: virDomainObjEndAPI(&vm); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index e911d88b56..e3849e00f0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -671,10 +671,8 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, } else { VIR_DEBUG("Stop event has already been sent"); } - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } } else { int ret = virLXCProcessReboot(driver, vm); virDomainAuditStop(vm, "reboot"); @@ -685,15 +683,15 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, priv->stopReason); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } } } - if (vm) - virObjectUnlock(vm); + /* NB: virLXCProcessConnectMonitor will perform the virObjectRef(vm) + * before adding monitorCallbacks. Since we are now done with the @vm + * we can Unref/Unlock */ + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(driver->domainEventState, event); } @@ -803,7 +801,8 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver, goto cleanup; /* Hold an extra reference because we can't allow 'vm' to be - * deleted while the monitor is active */ + * deleted while the monitor is active. This will be unreffed + * during EOFNotify processing. */ virObjectRef(vm); monitor = virLXCMonitorNew(vm, cfg->stateDir, &monitorCallbacks); diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index a25eaf570e..c2eda94f1c 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -611,7 +611,7 @@ int openvzLoadDomains(struct openvz_driver *driver) /* XXX OpenVZ doesn't appear to have concept of a transient domain */ dom->persistent = 1; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); dom = NULL; def = NULL; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index c10d6df663..14295dfda0 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -982,8 +982,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla cleanup: virDomainDefFree(vmdef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); openvzDriverUnlock(driver); return dom; } @@ -1071,8 +1070,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, cleanup: virDomainDefFree(vmdef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); openvzDriverUnlock(driver); return dom; } @@ -1151,12 +1149,10 @@ openvzDomainUndefineFlags(virDomainPtr dom, if (virRun(prog, NULL) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -2232,16 +2228,13 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, error: virDomainDefFree(def); - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } done: VIR_FREE(my_hostname); virURIFree(uri); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2396,7 +2389,6 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, VIR_DEBUG("Domain '%s' successfully migrated", vm->def->name); virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); ret = 0; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 326c939c85..b0f72c4a62 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7167,24 +7167,9 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, VIR_FREE(snapDir); } - virObjectRef(vm); - virDomainObjListRemove(driver->domains, vm); - /* - * virDomainObjListRemove() leaves the domain unlocked so it can - * be unref'd for other drivers that depend on that, but we still - * need to reset a job and we have a reference from the API that - * called this function. So we need to lock it back. This is - * just a workaround for the qemu driver. - * - * XXX: Ideally, the global handling of domain objects and object - * lists would be refactored so we don't need hacks like - * this, but since that requires refactor of all drivers, - * it's a work for another day. - */ - virObjectLock(vm); + virObjectUnref(cfg); - virObjectUnref(vm); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7484b00e23..d67ef93fde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1763,7 +1763,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, @@ -6735,7 +6734,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (flags & VIR_DOMAIN_SAVE_RUNNING) @@ -7409,9 +7407,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, driver->xmlopt, 0, &oldDef))) goto cleanup; - - virObjectRef(vm); def = NULL; + if (qemuDomainHasBlockjob(vm, true)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", _("domain has active block job")); @@ -16407,7 +16404,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 88b8253fa9..d4bbbe4c84 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2279,9 +2279,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a1888c0c9f..4c701ca7d0 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -893,7 +893,7 @@ testParseDomains(testDriverPtr privconn, int num, ret = -1; size_t i; xmlNodePtr *nodes = NULL; - virDomainObjPtr obj; + virDomainObjPtr obj = NULL; num = virXPathNodeSet("/node/domain", ctxt, &nodes); if (num < 0) @@ -921,10 +921,8 @@ testParseDomains(testDriverPtr privconn, goto error; } - if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) { - virObjectUnlock(obj); + if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) goto error; - } nsdata = def->namespaceData; obj->persistent = !nsdata->transient; @@ -932,20 +930,19 @@ testParseDomains(testDriverPtr privconn, if (nsdata->runstate != VIR_DOMAIN_SHUTOFF) { if (testDomainStartState(privconn, obj, - VIR_DOMAIN_RUNNING_BOOTED) < 0) { - virObjectUnlock(obj); + VIR_DOMAIN_RUNNING_BOOTED) < 0) goto error; - } } else { testDomainShutdownState(NULL, obj, 0); } virDomainObjSetState(obj, nsdata->runstate, 0); - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); } ret = 0; error: + virDomainObjEndAPI(&obj); VIR_FREE(nodes); return ret; } @@ -1678,10 +1675,8 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) { - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(privconn->domains, dom); - dom = NULL; - } goto cleanup; } @@ -1692,8 +1687,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); virDomainDefFree(def); testDriverUnlock(privconn); @@ -1787,10 +1781,8 @@ static int testDomainDestroyFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -1888,10 +1880,8 @@ static int testDomainShutdownFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -1957,10 +1947,8 @@ static int testDomainReboot(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } } ret = 0; @@ -2095,10 +2083,8 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -2200,10 +2186,8 @@ testDomainRestoreFlags(virConnectPtr conn, def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) { - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(privconn->domains, dom); - dom = NULL; - } goto cleanup; } @@ -2216,8 +2200,7 @@ testDomainRestoreFlags(virConnectPtr conn, virDomainDefFree(def); VIR_FREE(xml); VIR_FORCE_CLOSE(fd); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); return ret; } @@ -2280,10 +2263,8 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain, event = virDomainEventLifecycleNewFromObj(privdom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } } ret = 0; @@ -2794,8 +2775,7 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); virDomainDefFree(oldDef); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); return ret; } @@ -3062,12 +3042,10 @@ static int testDomainUndefineFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); privdom->hasManagedSave = false; - if (virDomainObjIsActive(privdom)) { + if (virDomainObjIsActive(privdom)) privdom->persistent = 0; - } else { + else virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b50ba1ba64..fd109dcd9c 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -379,10 +379,8 @@ 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)) { @@ -412,10 +410,8 @@ 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, @@ -424,10 +420,8 @@ 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); @@ -785,10 +779,8 @@ static int umlProcessAutoDestroyDom(void *payload, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(data->driver->domains, dom); - virObjectLock(dom); - } virDomainObjEndAPI(&dom); if (event) @@ -1609,10 +1601,8 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, if (umlStartVMDaemon(conn, driver, vm, (flags & VIR_DOMAIN_START_AUTODESTROY)) < 0) { virDomainAuditStart(vm, "booted", false); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } virDomainAuditStart(vm, "booted", true); @@ -1624,8 +1614,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, cleanup: virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -1694,10 +1683,8 @@ umlDomainDestroyFlags(virDomainPtr dom, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; cleanup: @@ -2009,7 +1996,6 @@ 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); - vm = NULL; goto cleanup; } @@ -2017,8 +2003,7 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) cleanup: virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return dom; } @@ -2054,12 +2039,10 @@ static int umlDomainUndefineFlags(virDomainPtr dom, if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index b9f18e6ac4..e1d6956df0 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -186,10 +186,9 @@ vmwareLoadDomains(struct vmware_driver *driver) VIR_DOMAIN_RUNNING_UNKNOWN); vm->persistent = 1; - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmdef = NULL; - vm = NULL; } ret = 0; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 21c10b6605..f94b3252fe 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -502,10 +502,8 @@ vmwareDomainShutdownFlags(virDomainPtr dom, if (vmwareStopVM(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN) < 0) goto cleanup; - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; cleanup: @@ -717,10 +715,8 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, vmdef = NULL; if (vmwareStartVM(driver, vm) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } @@ -730,8 +726,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, virDomainDefFree(vmdef); VIR_FREE(vmx); VIR_FREE(vmxPath); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmwareDriverUnlock(driver); return dom; } @@ -796,12 +791,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index d3fcae491a..48a9a866d9 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3161,7 +3161,6 @@ vzDomainMigratePerformStep(virDomainObjPtr dom, goto cleanup; virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); ret = 0; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index bb8f15e4fe..cb82b9d582 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1969,7 +1969,6 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; } - virObjectRef(dom); pdom = dom->privateData; pdom->sdkdom = sdkdom; PrlHandle_AddRef(sdkdom); @@ -2235,7 +2234,6 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); virDomainObjEndAPI(&dom); return; } @@ -4334,7 +4332,6 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); ret = 0; cleanup: -- 2.13.6
On Tue, Apr 24, 2018 at 08:28:09AM -0400, John Ferlan wrote:
When adding a new object to the domain object list, there should have been 2 virObjectRef calls made one for each list into which the object was placed to match the 2 virObjectUnref calls that would occur during Remove as part of virHashRemoveEntry when virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew.
Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency by calling virObjectRef upon successful return from virDomainObjListAdd in order to use virDomainObjEndAPI when done with the returned @vm. While others (bhyve, openvz, test, and vmware) handled this via only calling virObjectUnlock upon successful return from virDomainObjListAdd.
This patch will "unify" the approach to use virDomainObjEndAPI for any @vm successfully returned from virDomainObjListAdd.
Because list removal is so tightly coupled with list addition, this patch fixes the list removal algorithm to return the object as entered - "locked and reffed". This way, the callers can then decide how to uniformly handle add/remove success and failure. This removes the onus on the caller to "specially handle" the @vm during removal processing.
The Add/Remove logic allows for some logic simplification such as in libxl where we can Remove the @vm directly rather than needing to set a @remove_dom boolean and removing after the libxlDomainObjEndJob completes as the @vm is locked/reffed.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
...
@@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, * no one else is either waiting for 'dom' or still using it. * * When this function returns, @dom will be removed from the hash - * tables, unlocked, and returned with the refcnt that was present - * upon entry. + * tables and returned with lock and refcnt that was present upon entry.
I know ^this is pre-existing, but I reads strange, when we get here, refcnt is 3 (after your patches), when we get out, refcnt is 1, I know what the idea is, but it can be confusing, it sounds like the refcnt isn't changing when in fact it is. I went through this twice and figured that even if we missed something somewhere, it's so early in the new release-cycle that we have plenty of time to catch it, the changes look very good and it's an improvement. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 05/03/2018 12:00 PM, Erik Skultety wrote:
On Tue, Apr 24, 2018 at 08:28:09AM -0400, John Ferlan wrote:
When adding a new object to the domain object list, there should have been 2 virObjectRef calls made one for each list into which the object was placed to match the 2 virObjectUnref calls that would occur during Remove as part of virHashRemoveEntry when virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew.
Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency by calling virObjectRef upon successful return from virDomainObjListAdd in order to use virDomainObjEndAPI when done with the returned @vm. While others (bhyve, openvz, test, and vmware) handled this via only calling virObjectUnlock upon successful return from virDomainObjListAdd.
This patch will "unify" the approach to use virDomainObjEndAPI for any @vm successfully returned from virDomainObjListAdd.
Because list removal is so tightly coupled with list addition, this patch fixes the list removal algorithm to return the object as entered - "locked and reffed". This way, the callers can then decide how to uniformly handle add/remove success and failure. This removes the onus on the caller to "specially handle" the @vm during removal processing.
The Add/Remove logic allows for some logic simplification such as in libxl where we can Remove the @vm directly rather than needing to set a @remove_dom boolean and removing after the libxlDomainObjEndJob completes as the @vm is locked/reffed.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
...
@@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, * no one else is either waiting for 'dom' or still using it. * * When this function returns, @dom will be removed from the hash - * tables, unlocked, and returned with the refcnt that was present - * upon entry. + * tables and returned with lock and refcnt that was present upon entry.
I know ^this is pre-existing, but I reads strange, when we get here, refcnt is 3 (after your patches), when we get out, refcnt is 1, I know what the idea is, but it can be confusing, it sounds like the refcnt isn't changing when in fact it is.
Yeah - I'm not sure of the best way to describe this... BTW: This is the issue being fixed... If a driver didn't perform a virObjectRef after the ListAdd, then when ListRemove is called, we return w/ refcnt == 0 and those callers "knew" that and would be sure to set @vm = NULL after ListRemove... For those drivers that had the virObjectRef after ListAdd, we returned with a refcnt >= 1, without the lock, and we'd (ahem) re-lock (in some instances) or for others there was an Unref... Oh and I think along the way there were a few that used EndAPI, which would Unlock something that wasn't locked any more and Unref. The reason I noted >= 1 is that @vm can be in other structures, but those would (hopefully) have done the virObjectRef when placed there. There was the libxl one in patch 4, plus there's some others dealing with close callbacks and qemu_process event processing.
I went through this twice and figured that even if we missed something somewhere, it's so early in the new release-cycle that we have plenty of time to catch it, the changes look very good and it's an improvement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Great - thanks. Kind of my sentiments at least with respect to why waiting until 4.4.0 opened was perfectly reasonable for this change. I believe I've covered everything, but those are famous last words when it comes to bizarre corner cases. Tks - John
ping? Still need review on 1-3, & 6 Tks John On 04/24/2018 08:28 AM, John Ferlan wrote:
This should be the "last time" (famous last words) needing to alter this processing. I hope to get more than one pair of eyes looking at the series. I've looked at it long enough to feel comfortable with the changes, but I also know I probably have looked at it so long that I could have missed something!
The first 3 patches set up the necessary infrastructure. Patches 4 & 5 are just something I found in libxl while doing this - they could be extracted and considered separately if need be; however, since I was thinking about @vm referencing - it just seemed to fit. Since I altered @vm it only seemed right to do the same for @conn. While it could be said that neither condition could happen - I figured better safe than sorry and I think it follows how this type of logic has been handled elsewhere in qemu.
Patch 6 is where all the magic happens. I tried a few different ways to rework and split the Add/Remove processing, but either way I was left with rather ugly patches that required touching the same places, so I capitulated and combined them since there really is a delicate balance between those paths that have added a virObjectRef after the virDomainObjListAdd and those that just use virObjectUnlock on the returned ListAdd objects. Converting the latter to use Ref caused more "busy work" and equally large patches. The busy work was mosting around the processing required during ListAdd if something happened which resulted in ListRemove needing to be called.
The "general change" is that instead of having ListAdd return an object with 2 refs and 1 lock it will now return an object with 3 refs and 1 lock. With that returned object, some drivers would perform an ObjectRef while others did not. For those that did not, they would use ObjectUnlock when done with the object so that when leaving whatever Create routine there was there would be at least 2 Refs on the object. The change here is to have them all be consistent. What follows is a "general description" of what was done for each.
For bhyve, openvz, test, uml, and vmware (e.g. domain drivers that do not use virObjectRef on the returned virDomainObjListAdd object):
Create/Add processing:
-> Use EndAPI instead of ObjectUnlock -> Remove setting vm = NULL if the ListRemove was called to allow EndAPI processing to perform last Unref and Unlock
NB: ListRemove would remove 2 refs and unlock, so it "worked", but would "consume" the returned object
Undefine/Destroy processing:
-> Alter ListRemove and ObjectLock to just be ListRemove allowing the EndAPI processing from a FindBy* to perform last Unref and Unlock
For libxl, lxc, qemu, and vz (e.g. domain drivers that use virObjectRef on the returned virDomainObjListAdd object):
Create/Add processing:
-> Remove the ObjectRef -> Remove the ObjectLock when needing to call ListRemove to allow EndAPI processing to perform the last Unref and Unlock
NB: ListRemove no longer Unlock's the returned object
Undefine/Destroy processing:
-> Alter ListRemove and ObjectLock to just be ListRemove allowing the EndAPI processing from a FindBy* to perform last Unref and Unlock
NB: For the AutoDestroy type logic, no change is required since in the long run the object was left with the correct number of refs after create processing ran. The issue is more the direct mgmt of object during Add/Remove rather than mgmt of object once defined.
John Ferlan (6): conf: Split FindBy{UUID|Name} into locked helpers conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd conf: Move and use virDomainObjListRemoveLocked libxl: Add refcnt for args->vm during migration libxl: Add refcnt for args->conn during migration conf: Clean up object referencing for Add and Remove
src/bhyve/bhyve_driver.c | 21 ++---- src/conf/virdomainobjlist.c | 165 ++++++++++++++++++++++++++++---------------- src/libxl/libxl_driver.c | 64 ++++------------- src/libxl/libxl_migration.c | 41 ++++------- src/lxc/lxc_driver.c | 21 ++---- src/lxc/lxc_process.c | 17 +++-- src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 20 ++---- src/qemu/qemu_domain.c | 17 +---- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_migration.c | 3 +- src/test/test_driver.c | 56 +++++---------- src/uml/uml_driver.c | 37 +++------- src/vmware/vmware_conf.c | 3 +- src/vmware/vmware_driver.c | 17 ++--- src/vz/vz_driver.c | 1 - src/vz/vz_sdk.c | 3 - 17 files changed, 192 insertions(+), 302 deletions(-)
participants (3)
-
Erik Skultety -
John Ferlan -
Marc Hartmayer