From 0d1840729f154c956f0726789dba1cb6b79d28ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 15 May 2020 16:36:00 +0100 Subject: [PATCH] src: make virObjectUnref return void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To prepare for a conversion to GObject, we need virObjectUnref to have the same API design as g_object_unref, which means it needs to be void. A few places do actually care about the return value though, and in these cases a thread local flag is used to determine if the dispose method was invoked. Reviewed-by: Michal Privoznik Signed-off-by: Daniel P. Berrangé --- src/admin/libvirt-admin.c | 4 +++- src/datatypes.c | 26 +++++++++++++++++++++++++ src/datatypes.h | 6 ++++++ src/interface/interface_backend_netcf.c | 7 +------ src/libvirt.c | 4 +++- src/qemu/qemu_domain.c | 4 +++- src/qemu/qemu_monitor.c | 14 +++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/test/test_driver.c | 8 ++++++-- src/util/virfdstream.c | 6 +++++- src/util/virobject.c | 9 ++------- src/util/virobject.h | 2 +- src/vbox/vbox_common.c | 10 ++++++++-- 13 files changed, 81 insertions(+), 22 deletions(-) diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c index 835b5560d2..1d4ac51296 100644 --- a/src/admin/libvirt-admin.c +++ b/src/admin/libvirt-admin.c @@ -295,7 +295,9 @@ virAdmConnectClose(virAdmConnectPtr conn) virCheckAdmConnectReturn(conn, -1); - if (!virObjectUnref(conn)) + virAdmConnectWatchDispose(); + virObjectUnref(conn); + if (virAdmConnectWasDisposed()) return 0; return 1; } diff --git a/src/datatypes.c b/src/datatypes.c index 552115c7a3..1db38c5aa6 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -76,6 +76,9 @@ virClassPtr virAdmClientClass; static void virAdmServerDispose(void *obj); static void virAdmClientDispose(void *obj); +static __thread bool connectDisposed; +static __thread bool admConnectDisposed; + static int virDataTypesOnceInit(void) { @@ -133,6 +136,27 @@ virGetConnect(void) return virObjectLockableNew(virConnectClass); } + +void virConnectWatchDispose(void) +{ + connectDisposed = false; +} + +bool virConnectWasDisposed(void) +{ + return connectDisposed; +} + +void virAdmConnectWatchDispose(void) +{ + admConnectDisposed = false; +} + +bool virAdmConnectWasDisposed(void) +{ + return admConnectDisposed; +} + /** * virConnectDispose: * @obj: the hypervisor connection to release @@ -145,6 +169,7 @@ virConnectDispose(void *obj) { virConnectPtr conn = obj; + connectDisposed = true; if (conn->driver) conn->driver->connectClose(conn); @@ -1092,6 +1117,7 @@ virAdmConnectDispose(void *obj) { virAdmConnectPtr conn = obj; + admConnectDisposed = true; if (conn->privateDataFreeFunc) conn->privateDataFreeFunc(conn); diff --git a/src/datatypes.h b/src/datatypes.h index 2d0407f7ec..d58429ad6c 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -843,6 +843,12 @@ virAdmClientPtr virAdmGetClient(virAdmServerPtr srv, unsigned long long timestamp, unsigned int transport); +/* Thread local to watch if an ObjectUnref causes a Dispoe */ +void virConnectWatchDispose(void); +bool virConnectWasDisposed(void); +void virAdmConnectWatchDispose(void); +bool virAdmConnectWasDisposed(void); + virConnectCloseCallbackDataPtr virNewConnectCloseCallbackData(void); void virConnectCloseCallbackDataRegister(virConnectCloseCallbackDataPtr close, virConnectPtr conn, diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index dd0c1481d9..f30829442d 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -148,12 +148,7 @@ netcfStateCleanup(void) if (!driver) return -1; - if (virObjectUnref(driver)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Attempt to close netcf state driver " - "with open connections")); - return -1; - } + virObjectUnref(driver); driver = NULL; return 0; } diff --git a/src/libvirt.c b/src/libvirt.c index 76bf1fa677..b2d0ba3d23 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1277,7 +1277,9 @@ virConnectClose(virConnectPtr conn) virCheckConnectReturn(conn, -1); - if (!virObjectUnref(conn)) + virConnectWatchDispose(); + virObjectUnref(conn); + if (virConnectWasDisposed()) return 0; return 1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d5e3d1a3cc..10f16e9b3b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6691,8 +6691,10 @@ qemuDomainObjExitMonitorInternal(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = obj->privateData; bool hasRefs; - hasRefs = virObjectUnref(priv->mon); + qemuMonitorWatchDispose(); + virObjectUnref(priv->mon); + hasRefs = !qemuMonitorWasDisposed(); if (hasRefs) virObjectUnlock(priv->mon); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2911307f0e..b88eac96a1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -146,6 +146,7 @@ struct _qemuMonitor { QEMU_CHECK_MONITOR_FULL(mon, goto label) static virClassPtr qemuMonitorClass; +static __thread bool qemuMonitorDisposed; static void qemuMonitorDispose(void *obj); static int qemuMonitorOnceInit(void) @@ -222,6 +223,7 @@ qemuMonitorDispose(void *obj) qemuMonitorPtr mon = obj; VIR_DEBUG("mon=%p", mon); + qemuMonitorDisposed = true; if (mon->cb && mon->cb->destroy) (mon->cb->destroy)(mon, mon->vm, mon->callbackOpaque); virObjectUnref(mon->vm); @@ -799,6 +801,18 @@ qemuMonitorOpen(virDomainObjPtr vm, } +void qemuMonitorWatchDispose(void) +{ + qemuMonitorDisposed = false; +} + + +bool qemuMonitorWasDisposed(void) +{ + return qemuMonitorDisposed; +} + + /** * qemuMonitorRegister: * @mon: QEMU monitor diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c61c05cb9f..50a337715d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -387,6 +387,9 @@ qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, void *opaque) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); +void qemuMonitorWatchDispose(void); +bool qemuMonitorWasDisposed(void); + void qemuMonitorRegister(qemuMonitorPtr mon) ATTRIBUTE_NONNULL(1); void qemuMonitorUnregister(qemuMonitorPtr mon) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0506147888..3a085003e2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -128,6 +128,7 @@ static testDriverPtr defaultPrivconn; static virMutex defaultLock = VIR_MUTEX_INITIALIZER; static virClassPtr testDriverClass; +static __thread bool testDriverDisposed; static void testDriverDispose(void *obj); static int testDriverOnceInit(void) { @@ -171,6 +172,8 @@ testDriverDispose(void *obj) g_free(driver->auths[i].password); } g_free(driver->auths); + + testDriverDisposed = true; } typedef struct _testDomainNamespaceDef testDomainNamespaceDef; @@ -1446,8 +1449,9 @@ static void testDriverCloseInternal(testDriverPtr driver) { virMutexLock(&defaultLock); - bool disposed = !virObjectUnref(driver); - if (disposed && driver == defaultPrivconn) + testDriverDisposed = false; + virObjectUnref(driver); + if (testDriverDisposed && driver == defaultPrivconn) defaultPrivconn = NULL; virMutexUnlock(&defaultLock); } diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 111e451f8c..1c32be47a9 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -112,6 +112,7 @@ struct virFDStreamData { }; static virClassPtr virFDStreamDataClass; +static __thread bool virFDStreamDataDisposed; static void virFDStreamMsgQueueFree(virFDStreamMsgPtr *queue); @@ -121,6 +122,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj; VIR_DEBUG("obj=%p", fdst); + virFDStreamDataDisposed = true; virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); } @@ -631,7 +633,9 @@ virFDStreamThread(void *opaque) cleanup: fdst->threadQuit = true; virObjectUnlock(fdst); - if (!virObjectUnref(fdst)) + virFDStreamDataDisposed = false; + virObjectUnref(fdst); + if (virFDStreamDataDisposed) st->privateData = NULL; VIR_FORCE_CLOSE(fdin); VIR_FORCE_CLOSE(fdout); diff --git a/src/util/virobject.c b/src/util/virobject.c index c71781550f..4060d7307b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -331,17 +331,14 @@ virObjectRWLockableDispose(void *anyobj) * it hits zero, runs the "dispose" callbacks associated * with the object class and its parents before freeing * @anyobj. - * - * Returns true if the remaining reference count is - * non-zero, false if the object was disposed of */ -bool +void virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj; if (VIR_OBJECT_NOTVALID(obj)) - return false; + return; bool lastRef = !!g_atomic_int_dec_and_test(&obj->u.s.refs); PROBE(OBJECT_UNREF, "obj=%p", obj); @@ -360,8 +357,6 @@ virObjectUnref(void *anyobj) obj->klass = (void*)0xDEADBEEF; VIR_FREE(obj); } - - return !lastRef; } diff --git a/src/util/virobject.h b/src/util/virobject.h index 62a8a3d132..cfedb19b13 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -106,7 +106,7 @@ void * virObjectNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); -bool +void virObjectUnref(void *obj); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virObject, virObjectUnref); diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index e98ae04ec0..a834a971f0 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -61,6 +61,7 @@ static virClassPtr vboxDriverClass; static virMutex vbox_driver_lock = VIR_MUTEX_INITIALIZER; static vboxDriverPtr vbox_driver; static vboxDriverPtr vboxDriverObjNew(void); +static __thread bool vboxDriverDisposed; static int vboxDomainDevicesDefPostParse(virDomainDeviceDefPtr dev G_GNUC_UNUSED, @@ -124,6 +125,7 @@ vboxDriverDispose(void *obj) { vboxDriverPtr driver = obj; + vboxDriverDisposed = true; virObjectUnref(driver->caps); virObjectUnref(driver->xmlopt); } @@ -250,7 +252,9 @@ vboxGetDriverConnection(void) if (vboxSdkInitialize() < 0 || vboxExtractVersion() < 0) { gVBoxAPI.UPFN.Uninitialize(vbox_driver); /* make sure to clear the pointer when last reference was released */ - if (!virObjectUnref(vbox_driver)) + vboxDriverDisposed = false; + virObjectUnref(vbox_driver); + if (vboxDriverDisposed) vbox_driver = NULL; virMutexUnlock(&vbox_driver_lock); @@ -277,7 +281,9 @@ vboxDestroyDriverConnection(void) vboxSdkUninitialize(); - if (!virObjectUnref(vbox_driver)) + vboxDriverDisposed = false; + virObjectUnref(vbox_driver); + if (vboxDriverDisposed) vbox_driver = NULL; cleanup: