Re: [libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
On 03/13/2018 01:26 PM, Jim Fehlig wrote:
libxlDomainMigrationPrepare adds the incoming domain def to the list
of
domains via virDomainObjListAdd, which returns a locked and ref counted
virDomainObj. On exit the object is unlocked but not unref'ed. The same
is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the
virDomainObjEndAPI function for cleanup.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_migration.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
These two leave me concerned - mainly because as I described in my
review of 2/4 the ListAdd function will return 2 refs and 1 lock on the
object unlike the libxlDomObjFromDomain where there are at least 3 refs
and 1 lock. Since neither of these code paths adds a Ref(vm) after the
ListAdd call (like CreateXML and RestoreFlags do) that means calling
EndAPI will remove 1 ref and the lock leaving only 1 ref on the object.
Later on when ListRemove is called it would enter with 2 refs and 1 lock
and leave with @vm being destroyed. Not having a clear picture of all
the paths a @vm could have in libxl makes me concerned because there are
a few places where ListRemove is called *and* @vm is referenced
afterwards such as libxlDomainDestroyFlags
I think once ListAdd returns with the right number of refs, then yes,
this is the proper adjustment, but for now unless there's an extra Ref
done after the ListAdd, then we should leave things as is.
Thoughts? And does this make sense?
John
diff --git a/src/libxl/libxl_migration.c
b/src/libxl/libxl_migration.c
index fef1c998b..6b53f9587 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -639,9 +639,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
}
done:
- if (vm)
- virObjectUnlock(vm);
-
+ virDomainObjEndAPI(&vm);
return ret;
}
@@ -820,8 +818,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
VIR_FREE(hostname);
else
virURIFree(uri);
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
}