[libvirt] [PATCH v2 0/3] Clean/fix test driver usage of FindBy{UUID|ID}
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg00507.html e.g., part of a 20 patch series: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html Changes since v1: * Add patches 2 and 3 essentially... Doing the same ByIDRef as ByUUIDRef and fixing the ListRemove consumers to do the Lock. NB: The ByIDRef logic was formerly part of patch 20 - this just extracts it a bit to make it more orderly. John Ferlan (3): test: Use virDomainObjListFindByUUIDRef test: Use virDomainObjListFindByIDRef test: Need to relock afer virDomainObjListRemove src/test/test_driver.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) -- 2.13.6
Rather than using virDomainObjListFindByUUID, let's be more consistent and return a reffed and locked object. Since we're using the Ref API, use virDomainObjEndAPI on @dom and not just virObjectUnlock. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 99c27cc0a0..0cacab2fde 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1740,16 +1740,14 @@ static virDomainPtr testDomainLookupByUUID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom; - if (!(dom = virDomainObjListFindByUUID(privconn->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(privconn->domains, uuid))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; + return NULL; } ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); - cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } -- 2.13.6
On Mon, Apr 02, 2018 at 02:27 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
Rather than using virDomainObjListFindByUUID, let's be more consistent and return a reffed and locked object. Since we're using the Ref API, use virDomainObjEndAPI on @dom and not just virObjectUnlock.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 99c27cc0a0..0cacab2fde 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1740,16 +1740,14 @@ static virDomainPtr testDomainLookupByUUID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom;
- if (!(dom = virDomainObjListFindByUUID(privconn->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(privconn->domains, uuid))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; + return NULL; }
ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
- cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Rather than using virDomainObjListFindByID, let's be more consistent and return a reffed and locked object. Since we're using the Ref API, use virDomainObjEndAPI on @dom and not just virObjectUnlock. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0cacab2fde..3c01aa50e1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1720,16 +1720,14 @@ static virDomainPtr testDomainLookupByID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom; - if (!(dom = virDomainObjListFindByID(privconn->domains, id))) { + if (!(dom = virDomainObjListFindByIDRef(privconn->domains, id))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; + return NULL; } ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); - cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } -- 2.13.6
On Mon, Apr 02, 2018 at 02:27 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
Rather than using virDomainObjListFindByID, let's be more consistent and return a reffed and locked object. Since we're using the Ref API, use virDomainObjEndAPI on @dom and not just virObjectUnlock.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0cacab2fde..3c01aa50e1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1720,16 +1720,14 @@ static virDomainPtr testDomainLookupByID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom;
- if (!(dom = virDomainObjListFindByID(privconn->domains, id))) { + if (!(dom = virDomainObjListFindByIDRef(privconn->domains, id))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; + return NULL; }
ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
- cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
For all @dom's fetched from a testDomObjFromDomain because virDomainObjListRemove will return an unlocked domain object we should relock it prior to the cleanup label which will use virDomainObjEndAPI which would Unlock and Unref the passed object (and we should avoid unlocking an unlocked object). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3c01aa50e1..5e39ae574c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1802,8 +1802,10 @@ 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: @@ -1901,8 +1903,10 @@ 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: @@ -1971,8 +1975,10 @@ 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; @@ -2110,8 +2116,10 @@ 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: @@ -2296,8 +2304,10 @@ 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; @@ -3076,10 +3086,12 @@ 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; -- 2.13.6
On Mon, Apr 02, 2018 at 02:27 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
For all @dom's fetched from a testDomObjFromDomain because virDomainObjListRemove will return an unlocked domain object we should relock it prior to the cleanup label which will use virDomainObjEndAPI which would Unlock and Unref the passed object (and we should avoid unlocking an unlocked object).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3c01aa50e1..5e39ae574c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1802,8 +1802,10 @@ 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: @@ -1901,8 +1903,10 @@ 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: @@ -1971,8 +1975,10 @@ 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; @@ -2110,8 +2116,10 @@ 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: @@ -2296,8 +2304,10 @@ 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; @@ -3076,10 +3086,12 @@ 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;
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> I would change the order of this patch series as the last patch is a fix, no? -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 04/04/2018 02:57 AM, Marc Hartmayer wrote:
On Mon, Apr 02, 2018 at 02:27 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
For all @dom's fetched from a testDomObjFromDomain because virDomainObjListRemove will return an unlocked domain object we should relock it prior to the cleanup label which will use virDomainObjEndAPI which would Unlock and Unref the passed object (and we should avoid unlocking an unlocked object).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3c01aa50e1..5e39ae574c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1802,8 +1802,10 @@ 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: @@ -1901,8 +1903,10 @@ 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: @@ -1971,8 +1975,10 @@ 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; @@ -2110,8 +2116,10 @@ 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: @@ -2296,8 +2304,10 @@ 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; @@ -3076,10 +3086,12 @@ 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;
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
I would change the order of this patch series as the last patch is a fix, no?
I can move it to be first as it's not related to the other two. Thanks for the reviews! John
-- 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
participants (2)
-
John Ferlan -
Marc Hartmayer