[libvirt] [PATCH 0/4] Initial phase of domain obj add/remove cleanup
As evidenced by various code comments, the process of adding and removing objects to/from the domain object list is problematic. Long story short is that the Add logic doesn't generate enough object references and the Remove logic removes one extra than was added during Add and leaves the object unlocked upon return (as well as doing a small fire dance to ensure proper lock ordering). Some drivers (libxl, lxc, qemu, and vz) handle the not enough references by adding an virObjectRef to the object returned from the Add code, while others (bhyve, openvz, test, uml, and vmware) live rather vicariously and carefully, but at least don't reference the object after calling Remove. Fixing all this will take a few patch streams across a few mostly dormant driver modules and some coordination with the vir*FindBy{UUID|ID|Name} logic. Some of that was already posted previously, but only received minimal notice: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html So rather than (re)posting a 20-30 patch series on list which probably won't get reviewed, I'll take things in smaller batches of patches in the hopes that all this can be worked through so that the end result is "cleaner" (and agreed upon). John Ferlan (4): conf: Fix error path logic in virDomainObjListAddLocked conf: Fix error path logic in virDomainObjListLoadStatus conf: Introduce virDomainObjListAddObjLocked conf: Fix virDomainObjParseFile object handling src/conf/virdomainobjlist.c | 63 ++++++++++++++++++++++++++++----------------- src/lxc/lxc_controller.c | 2 +- tests/qemuxml2xmltest.c | 2 +- 3 files changed, 42 insertions(+), 25 deletions(-) -- 2.13.6
If the virHashAddEntry fails, then we need to "careful" about how we free the @vm. When virDomainObjNew returns there is one reference and the object is locked, so use virDomainObjEndAPI when done. Add a virObjectRef in the error path for the second virHashAddEntry call since it doesn't call virObjectRef, but virHashRemoveEntry will call virObjectUnref because virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew. Eventually these paths should goto error and error should be changed to use EndAPI as well, but that requires more adjustments to other paths in the code to have a locked and ref counted @vm. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 87a742b1e..2ae8a41a1 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) { - virObjectUnref(vm); + virObjectEndAPI(&vm); return NULL; } if (virHashAddEntry(doms->objsName, def->name, vm) < 0) { + virObjectRef(vm); virHashRemoveEntry(doms->objs, uuidstr); + virObjectEndAPI(&vm); return NULL; } -- 2.13.6
On 03/29/2018 02:34 PM, John Ferlan wrote:
If the virHashAddEntry fails, then we need to "careful" about how we free the @vm. When virDomainObjNew returns there is one reference and the object is locked, so use virDomainObjEndAPI when done.
Add a virObjectRef in the error path for the second virHashAddEntry call since it doesn't call virObjectRef, but virHashRemoveEntry will call virObjectUnref because virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew.
Eventually these paths should goto error and error should be changed to use EndAPI as well, but that requires more adjustments to other paths in the code to have a locked and ref counted @vm.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 87a742b1e..2ae8a41a1 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) { - virObjectUnref(vm); + virObjectEndAPI(&vm);
s/virObjectEndAPI/virDomainObjEndAPI/ here and down too ;-)
return NULL; }
if (virHashAddEntry(doms->objsName, def->name, vm) < 0) { + virObjectRef(vm); virHashRemoveEntry(doms->objs, uuidstr); + virObjectEndAPI(&vm); return NULL; }
ACK Michal
On 04/06/2018 08:56 AM, Michal Privoznik wrote:
On 03/29/2018 02:34 PM, John Ferlan wrote:
If the virHashAddEntry fails, then we need to "careful" about how we free the @vm. When virDomainObjNew returns there is one reference and the object is locked, so use virDomainObjEndAPI when done.
Add a virObjectRef in the error path for the second virHashAddEntry call since it doesn't call virObjectRef, but virHashRemoveEntry will call virObjectUnref because virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew.
Eventually these paths should goto error and error should be changed to use EndAPI as well, but that requires more adjustments to other paths in the code to have a locked and ref counted @vm.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 87a742b1e..2ae8a41a1 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) { - virObjectUnref(vm); + virObjectEndAPI(&vm);
s/virObjectEndAPI/virDomainObjEndAPI/ here and down too ;-)
Oh right - good catch... obviously I worked backwards from patch 3 but my fingers mistyped things Fixed... Tks, John
return NULL; }
if (virHashAddEntry(doms->objsName, def->name, vm) < 0) { + virObjectRef(vm); virHashRemoveEntry(doms->objs, uuidstr); + virObjectEndAPI(&vm); return NULL; }
ACK
Michal
If the virHashAddEntry fails, then we need to "careful" about how we free the @obj. When virDomainObjParseFile returns there is one reference and the object is locked, so use virDomainObjEndAPI when done. Add a virObjectRef in the error path for the second virHashAddEntry call since it doesn't call virObjectRef, but virHashRemoveEntry will call virObjectUnref because virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 2ae8a41a1..765f46d5a 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -536,6 +536,7 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, goto error; if (virHashAddEntry(doms->objsName, obj->def->name, obj) < 0) { + virObjectRef(obj); virHashRemoveEntry(doms->objs, uuidstr); goto error; } @@ -551,7 +552,7 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, return obj; error: - virObjectUnref(obj); + virDomainObjEndAPI(&obj); VIR_FREE(statusFile); return NULL; } -- 2.13.6
Create a common helper to add an object to the locked domain objlist hash tables and use it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 64 +++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 765f46d5a..7022abe09 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -220,6 +220,42 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, } +/** + * @doms: Domain object list pointer + * @vm: Domain object to be added + * + * Upon entry @vm should have at least 1 ref and be locked. + * + * Add the @vm into the @doms->objs and @doms->objsName hash + * tables. + * + * Returns 0 on success with 2 references and locked + * -1 on failure with 1 reference and locked + */ +static int +virDomainObjListAddObjLocked(virDomainObjListPtr doms, + virDomainObjPtr vm) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(vm->def->uuid, uuidstr); + if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) + return -1; + + 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; +} + + /* * virDomainObjListAddLocked: * @@ -294,22 +330,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, goto cleanup; vm->def = def; - virUUIDFormat(def->uuid, uuidstr); - if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) { - virObjectEndAPI(&vm); + if (virDomainObjListAddObjLocked(doms, vm) < 0) { + virDomainObjEndAPI(&vm); return NULL; } - - if (virHashAddEntry(doms->objsName, def->name, vm) < 0) { - virObjectRef(vm); - virHashRemoveEntry(doms->objs, uuidstr); - virObjectEndAPI(&vm); - return NULL; - } - - /* Since domain is in two hash tables, increment the - * reference counter */ - virObjectRef(vm); } cleanup: return vm; @@ -532,19 +556,9 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, goto error; } - if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) + if (virDomainObjListAddObjLocked(doms, obj) < 0) goto error; - if (virHashAddEntry(doms->objsName, obj->def->name, obj) < 0) { - virObjectRef(obj); - virHashRemoveEntry(doms->objs, uuidstr); - goto error; - } - - /* Since domain is in two hash tables, increment the - * reference counter */ - virObjectRef(obj); - if (notify) (*notify)(obj, 1, opaque); -- 2.13.6
On 03/29/2018 02:34 PM, John Ferlan wrote:
Create a common helper to add an object to the locked domain objlist hash tables and use it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 64 +++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 765f46d5a..7022abe09 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -220,6 +220,42 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, }
+/** + * @doms: Domain object list pointer + * @vm: Domain object to be added + * + * Upon entry @vm should have at least 1 ref and be locked. + * + * Add the @vm into the @doms->objs and @doms->objsName hash + * tables. + * + * Returns 0 on success with 2 references and locked + * -1 on failure with 1 reference and locked + */ +static int +virDomainObjListAddObjLocked(virDomainObjListPtr doms, + virDomainObjPtr vm) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(vm->def->uuid, uuidstr); + if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) + return -1; + + 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);
I think this virObjectRef() could be moved in between those two virHashAddEntry() calls and thus the later call to virObjectRef() can be dropped then. Michal
On 04/06/2018 08:56 AM, Michal Privoznik wrote:
On 03/29/2018 02:34 PM, John Ferlan wrote:
Create a common helper to add an object to the locked domain objlist hash tables and use it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 64 +++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 765f46d5a..7022abe09 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -220,6 +220,42 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, }
+/** + * @doms: Domain object list pointer + * @vm: Domain object to be added + * + * Upon entry @vm should have at least 1 ref and be locked. + * + * Add the @vm into the @doms->objs and @doms->objsName hash + * tables. + * + * Returns 0 on success with 2 references and locked + * -1 on failure with 1 reference and locked + */ +static int +virDomainObjListAddObjLocked(virDomainObjListPtr doms, + virDomainObjPtr vm) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(vm->def->uuid, uuidstr); + if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) + return -1; + + 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);
I think this virObjectRef() could be moved in between those two virHashAddEntry() calls and thus the later call to virObjectRef() can be dropped then.
True; however, the purpose of the first two patches was to set the code up to mirror each other and thus have this helper instead do the same thing. The reality is the next step *is* to put a virObjectRef between the two and thus have 2 virObjectRef calls to match the 2 Unref calls in the *Remove function. The next step also removes the comment leaving an ObjectRef after each successful AddEntry. Doing this means "some" drivers won't need the extra virObjectRef after they call the Add function, while "other" drivers will need to use the Remove and EndAPI after they call the Add function. I've been trying to coordinate this effort with the FindBy{UUID|ID}[Ref] effort as they are (in a way) strangely related. Tks- John
When virDomainObjParseFile runs, it returns a locked @obj with one reference. Rather than just use virObjectUnref to clean that up, use virObjectEndAPI. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_controller.c | 2 +- tests/qemuxml2xmltest.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 59609b0a0..507bffda0 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -284,7 +284,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) VIR_FREE(ctrl->devptmx); - virObjectUnref(ctrl->vm); + virDomainObjEndAPI(&ctrl->vm); VIR_FREE(ctrl->name); if (ctrl->timerShutdown != -1) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0f560290a..ead4be873 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -96,7 +96,7 @@ testCompareStatusXMLToXMLFiles(const void *opaque) ret = 0; cleanup: - virObjectUnref(obj); + virDomainObjEndAPI(&obj); VIR_FREE(actual); return ret; } -- 2.13.6
ping? Tks - John On 03/29/2018 08:34 AM, John Ferlan wrote:
As evidenced by various code comments, the process of adding and removing objects to/from the domain object list is problematic.
Long story short is that the Add logic doesn't generate enough object references and the Remove logic removes one extra than was added during Add and leaves the object unlocked upon return (as well as doing a small fire dance to ensure proper lock ordering). Some drivers (libxl, lxc, qemu, and vz) handle the not enough references by adding an virObjectRef to the object returned from the Add code, while others (bhyve, openvz, test, uml, and vmware) live rather vicariously and carefully, but at least don't reference the object after calling Remove.
Fixing all this will take a few patch streams across a few mostly dormant driver modules and some coordination with the vir*FindBy{UUID|ID|Name} logic. Some of that was already posted previously, but only received minimal notice:
https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
So rather than (re)posting a 20-30 patch series on list which probably won't get reviewed, I'll take things in smaller batches of patches in the hopes that all this can be worked through so that the end result is "cleaner" (and agreed upon).
John Ferlan (4): conf: Fix error path logic in virDomainObjListAddLocked conf: Fix error path logic in virDomainObjListLoadStatus conf: Introduce virDomainObjListAddObjLocked conf: Fix virDomainObjParseFile object handling
src/conf/virdomainobjlist.c | 63 ++++++++++++++++++++++++++++----------------- src/lxc/lxc_controller.c | 2 +- tests/qemuxml2xmltest.c | 2 +- 3 files changed, 42 insertions(+), 25 deletions(-)
On 03/29/2018 02:34 PM, John Ferlan wrote:
As evidenced by various code comments, the process of adding and removing objects to/from the domain object list is problematic.
Long story short is that the Add logic doesn't generate enough object references and the Remove logic removes one extra than was added during Add and leaves the object unlocked upon return (as well as doing a small fire dance to ensure proper lock ordering). Some drivers (libxl, lxc, qemu, and vz) handle the not enough references by adding an virObjectRef to the object returned from the Add code, while others (bhyve, openvz, test, uml, and vmware) live rather vicariously and carefully, but at least don't reference the object after calling Remove.
Fixing all this will take a few patch streams across a few mostly dormant driver modules and some coordination with the vir*FindBy{UUID|ID|Name} logic. Some of that was already posted previously, but only received minimal notice:
https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
So rather than (re)posting a 20-30 patch series on list which probably won't get reviewed, I'll take things in smaller batches of patches in the hopes that all this can be worked through so that the end result is "cleaner" (and agreed upon).
John Ferlan (4): conf: Fix error path logic in virDomainObjListAddLocked conf: Fix error path logic in virDomainObjListLoadStatus conf: Introduce virDomainObjListAddObjLocked conf: Fix virDomainObjParseFile object handling
src/conf/virdomainobjlist.c | 63 ++++++++++++++++++++++++++++----------------- src/lxc/lxc_controller.c | 2 +- tests/qemuxml2xmltest.c | 2 +- 3 files changed, 42 insertions(+), 25 deletions(-)
ACK series. Michal
participants (2)
-
John Ferlan -
Michal Privoznik