7:28 a.m.
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
7:28 a.m.
New subject: [libvirt] [PATCH 1/6] conf: Split FindBy{UUID|Name} into locked helpers
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(a)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
5:38 a.m.
New subject: [libvirt] [PATCH 1/6] conf: Split FindBy{UUID|Name} into locked helpers
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(a)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(a)redhat.com>
8:01 a.m.
New subject: [libvirt] [PATCH 1/6] conf: Split FindBy{UUID|Name} into locked helpers
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(a)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(a)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
7:28 a.m.
New subject: [libvirt] [PATCH 2/6] conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd
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(a)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
11:02 a.m.
New subject: [libvirt] [PATCH 2/6] conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd
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(a)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(a)redhat.com>
11:15 a.m.
New subject: [libvirt] [PATCH 2/6] conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd
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(a)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(a)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
7:28 a.m.
New subject: [libvirt] [PATCH 3/6] conf: Move and use virDomainObjListRemoveLocked
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(a)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
11:03 a.m.
New subject: [libvirt] [PATCH 3/6] conf: Move and use virDomainObjListRemoveLocked
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(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
7:28 a.m.
New subject: [libvirt] [PATCH 4/6] libxl: Add refcnt for args->vm during migration
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(a)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
6:55 a.m.
New subject: [libvirt] [PATCH 4/6] libxl: Add refcnt for args->vm during migration
On Tue, Apr 24, 2018 at 02:28 PM +0200, John Ferlan <jferlan(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay(a)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
7:28 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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(a)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
6:56 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
On Tue, Apr 24, 2018 at 02:28 PM +0200, John Ferlan <jferlan(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay(a)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
7:34 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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(a)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
8:02 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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(a)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
9 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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(a)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(a)redhat.com>
PS: I still think you could move 4,5 after 6, since to me the causality of
changes is more evident.
7:36 a.m.
New subject: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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(a)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
7:28 a.m.
New subject: [libvirt] [PATCH 6/6] conf: Clean up object referencing for Add and Remove
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(a)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
11 a.m.
New subject: [libvirt] [PATCH 6/6] conf: Clean up object referencing for Add and Remove
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(a)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(a)redhat.com>
11:33 a.m.
New subject: [libvirt] [PATCH 6/6] conf: Clean up object referencing for Add and Remove
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(a)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(a)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
6:54 p.m.
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(-)
2394
days inactive
2403
days old
20 comments
3 participants
participants (3)
-
Erik Skultety -
John Ferlan -
Marc Hartmayer