[libvirt PATCH 0/6] src: make virObject inherit from GObject
This series attempts to make the conversion from virObject to GObject less dangerous, by making the former inherit from the latter. The way we setup inheritance with virObject means we have to make some moderately gross games with registering virClass instances as GObjectClass instances. Empirically it seems to work and pass the basic TCK test suite. Ultimately virObject should be deleted entirely, so the gross bits shouldn't live too long. Daniel P. Berrangé (6): qemu: stop checking virObjectUnref return value src: make virObjectUnref return void test: allocate numa cells separately from driver src: don't include ref count in debug messages / probes src: don't use VIR_FREE on an object allocation src: make virObject inherit from GObject src/admin/libvirt-admin.c | 7 +- src/datatypes.c | 26 ++++ src/datatypes.h | 6 + src/interface/interface_backend_netcf.c | 7 +- src/libvirt-domain-checkpoint.c | 3 +- src/libvirt-domain-snapshot.c | 3 +- src/libvirt-domain.c | 2 +- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 2 +- src/libvirt-network.c | 6 +- src/libvirt-nodedev.c | 2 +- src/libvirt-nwfilter.c | 6 +- src/libvirt-secret.c | 3 +- src/libvirt-storage.c | 4 +- src/libvirt-stream.c | 3 +- src/libvirt.c | 4 +- src/libvirt_qemu_probes.d | 8 +- src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_monitor.c | 21 +++- src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_process.c | 30 ++--- src/rpc/virnettlscontext.c | 5 +- src/test/test_driver.c | 15 ++- src/util/virfdstream.c | 6 +- src/util/virobject.c | 150 ++++++++++++++---------- src/util/virobject.h | 27 ++--- src/vbox/vbox_common.c | 10 +- 27 files changed, 210 insertions(+), 155 deletions(-) -- 2.24.1
Some, but not all, of the monitor event handlers check the virObjectUnref return value to see if the domain was disposed. It should not be possible for this to happen, since the functional ready holds a lock on the domain and has only just acquired an extra reference on the domain a few lines earlier. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_process.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f7f6793113..51a086031d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -307,7 +307,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); qemuProcessEventFree(processEvent); goto cleanup; } @@ -840,15 +840,13 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon G_GNUC_UNUSED, */ processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - if (!virObjectUnref(vm)) - vm = NULL; + virObjectUnref(vm); qemuProcessEventFree(processEvent); } } } - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, watchdogEvent); virObjectEventStateQueue(driver->domainEventState, lifecycleEvent); @@ -977,7 +975,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->status = status; if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); goto cleanup; } @@ -1039,7 +1037,7 @@ qemuProcessHandleJobStatusChange(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->data = virObjectRef(job); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); goto cleanup; } @@ -1342,14 +1340,12 @@ qemuProcessHandleGuestPanic(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - if (!virObjectUnref(vm)) - vm = NULL; + virObjectUnref(vm); qemuProcessEventFree(processEvent); } cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return 0; } @@ -1383,7 +1379,7 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); goto error; } @@ -1554,7 +1550,7 @@ qemuProcessHandleNicRxFilterChanged(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); goto error; } @@ -1593,7 +1589,7 @@ qemuProcessHandleSerialChanged(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - ignore_value(virObjectUnref(vm)); + virObjectUnref(vm); goto error; } @@ -1873,14 +1869,12 @@ qemuProcessHandleGuestCrashloaded(qemuMonitorPtr mon G_GNUC_UNUSED, processEvent->vm = virObjectRef(vm); if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { - if (!virObjectUnref(vm)) - vm = NULL; + virObjectUnref(vm); qemuProcessEventFree(processEvent); } cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return 0; } -- 2.24.1
On 5/19/20 12:41 PM, Daniel P. Berrangé wrote:
Some, but not all, of the monitor event handlers check the virObjectUnref return value to see if the domain was disposed.
It should not be possible for this to happen, since the functional ready holds a lock on the domain and
s/functional ready/function already/ (not everyday that a misplaced space still results in two valid words which might still legitimately appear adjacent in some other sentence...)
has only just acquired an extra reference on the domain a few lines earlier.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_process.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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 d0528dbfe0..bb77cd38d3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6681,8 +6681,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 9c853ccb93..ff7d66eee5 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 2e35d94bda..99c73e14af 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: -- 2.24.1
GObject has an arbitrary limit on the object struct size of 0xffff bytes. It is expected that any large fields be separately allocated. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/test/test_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3a085003e2..e8bfcd78d2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -92,7 +92,8 @@ struct _testAuth { typedef struct _testAuth testAuth; typedef struct _testAuth *testAuthPtr; -struct _testDriver { +struct _testDriver +{ virObjectLockable parent; virNodeInfo nodeInfo; @@ -102,7 +103,7 @@ struct _testDriver { virStoragePoolObjListPtr pools; virNodeDeviceObjListPtr devs; int numCells; - testCell cells[MAX_CELLS]; + testCell *cells; size_t numAuths; testAuthPtr auths; @@ -171,6 +172,7 @@ testDriverDispose(void *obj) g_free(driver->auths[i].username); g_free(driver->auths[i].password); } + g_free(driver->cells); g_free(driver->auths); testDriverDisposed = true; @@ -1353,6 +1355,7 @@ testOpenDefault(virConnectPtr conn) /* Numa setup */ privconn->numCells = 2; + privconn->cells = g_new0(testCell, privconn->numCells); for (i = 0; i < privconn->numCells; i++) { privconn->cells[i].numCpus = 8; privconn->cells[i].mem = (i + 1) * 2048 * 1024; -- 2.24.1
The ref count will be private to the GObject base class and we must not peek at it, even for debugging messages. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- src/libvirt-domain-checkpoint.c | 3 +-- src/libvirt-domain-snapshot.c | 3 +-- src/libvirt-domain.c | 2 +- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 2 +- src/libvirt-network.c | 6 ++---- src/libvirt-nodedev.c | 2 +- src/libvirt-nwfilter.c | 6 ++---- src/libvirt-secret.c | 3 +-- src/libvirt-storage.c | 4 ++-- src/libvirt-stream.c | 3 +-- src/libvirt_qemu_probes.d | 8 ++++---- src/qemu/qemu_monitor.c | 7 ++----- 14 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c index 1d4ac51296..b7f21275cb 100644 --- a/src/admin/libvirt-admin.c +++ b/src/admin/libvirt-admin.c @@ -322,8 +322,7 @@ virAdmConnectClose(virAdmConnectPtr conn) int virAdmConnectRef(virAdmConnectPtr conn) { - VIR_DEBUG("conn=%p refs=%d", conn, - conn ? conn->parent.parent.u.s.refs : 0); + VIR_DEBUG("conn=%p", conn); virResetLastError(); virCheckAdmConnectReturn(conn, -1); diff --git a/src/libvirt-domain-checkpoint.c b/src/libvirt-domain-checkpoint.c index 432c2d5a52..50627c486c 100644 --- a/src/libvirt-domain-checkpoint.c +++ b/src/libvirt-domain-checkpoint.c @@ -535,8 +535,7 @@ virDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, int virDomainCheckpointRef(virDomainCheckpointPtr checkpoint) { - VIR_DEBUG("checkpoint=%p, refs=%d", checkpoint, - checkpoint ? checkpoint->parent.u.s.refs : 0); + VIR_DEBUG("checkpoint=%p", checkpoint); virResetLastError(); diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 33593e11e9..f856e5b9b8 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -1080,8 +1080,7 @@ virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, int virDomainSnapshotRef(virDomainSnapshotPtr snapshot) { - VIR_DEBUG("snapshot=%p, refs=%d", snapshot, - snapshot ? snapshot->parent.u.s.refs : 0); + VIR_DEBUG("snapshot=%p", snapshot); virResetLastError(); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 37f864b7b0..60b5e65fc3 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -590,7 +590,7 @@ virDomainFree(virDomainPtr domain) int virDomainRef(virDomainPtr domain) { - VIR_DOMAIN_DEBUG(domain, "refs=%d", domain ? domain->parent.u.s.refs : 0); + VIR_DOMAIN_DEBUG(domain); virResetLastError(); diff --git a/src/libvirt-host.c b/src/libvirt-host.c index bc3d1d2803..07d13585f4 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -51,7 +51,7 @@ VIR_LOG_INIT("libvirt.host"); int virConnectRef(virConnectPtr conn) { - VIR_DEBUG("conn=%p refs=%d", conn, conn ? conn->parent.parent.u.s.refs : 0); + VIR_DEBUG("conn=%p", conn); virResetLastError(); diff --git a/src/libvirt-interface.c b/src/libvirt-interface.c index 2d2df68131..5eb5980483 100644 --- a/src/libvirt-interface.c +++ b/src/libvirt-interface.c @@ -642,7 +642,7 @@ virInterfaceDestroy(virInterfacePtr iface, unsigned int flags) int virInterfaceRef(virInterfacePtr iface) { - VIR_DEBUG("iface=%p refs=%d", iface, iface ? iface->parent.u.s.refs : 0); + VIR_DEBUG("iface=%p", iface); virResetLastError(); diff --git a/src/libvirt-network.c b/src/libvirt-network.c index 09e24fb0a8..f691b672c7 100644 --- a/src/libvirt-network.c +++ b/src/libvirt-network.c @@ -679,8 +679,7 @@ virNetworkFree(virNetworkPtr network) int virNetworkRef(virNetworkPtr network) { - VIR_DEBUG("network=%p refs=%d", network, - network ? network->parent.u.s.refs : 0); + VIR_DEBUG("network=%p", network); virResetLastError(); @@ -1714,8 +1713,7 @@ virNetworkPortFree(virNetworkPortPtr port) int virNetworkPortRef(virNetworkPortPtr port) { - VIR_DEBUG("port=%p refs=%d", port, - port ? port->parent.u.s.refs : 0); + VIR_DEBUG("port=%p", port); virResetLastError(); diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index dce46b7181..cdec123568 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -477,7 +477,7 @@ virNodeDeviceFree(virNodeDevicePtr dev) int virNodeDeviceRef(virNodeDevicePtr dev) { - VIR_DEBUG("dev=%p refs=%d", dev, dev ? dev->parent.u.s.refs : 0); + VIR_DEBUG("dev=%p", dev); virResetLastError(); diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c index d28220db8a..e299385895 100644 --- a/src/libvirt-nwfilter.c +++ b/src/libvirt-nwfilter.c @@ -503,8 +503,7 @@ virNWFilterGetXMLDesc(virNWFilterPtr nwfilter, unsigned int flags) int virNWFilterRef(virNWFilterPtr nwfilter) { - VIR_DEBUG("nwfilter=%p refs=%d", nwfilter, - nwfilter ? nwfilter->parent.u.s.refs : 0); + VIR_DEBUG("nwfilter=%p", nwfilter); virResetLastError(); @@ -820,8 +819,7 @@ virNWFilterBindingGetXMLDesc(virNWFilterBindingPtr binding, unsigned int flags) int virNWFilterBindingRef(virNWFilterBindingPtr binding) { - VIR_DEBUG("binding=%p refs=%d", binding, - binding ? binding->parent.u.s.refs : 0); + VIR_DEBUG("binding=%p", binding); virResetLastError(); diff --git a/src/libvirt-secret.c b/src/libvirt-secret.c index 33cbdd7b0b..75d40f53dc 100644 --- a/src/libvirt-secret.c +++ b/src/libvirt-secret.c @@ -658,8 +658,7 @@ virSecretUndefine(virSecretPtr secret) int virSecretRef(virSecretPtr secret) { - VIR_DEBUG("secret=%p refs=%d", secret, - secret ? secret->parent.u.s.refs : 0); + VIR_DEBUG("secret=%p", secret); virResetLastError(); diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 0406fe84d3..a45c8b98c1 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -872,7 +872,7 @@ virStoragePoolFree(virStoragePoolPtr pool) int virStoragePoolRef(virStoragePoolPtr pool) { - VIR_DEBUG("pool=%p refs=%d", pool, pool ? pool->parent.u.s.refs : 0); + VIR_DEBUG("pool=%p", pool); virResetLastError(); @@ -1909,7 +1909,7 @@ virStorageVolFree(virStorageVolPtr vol) int virStorageVolRef(virStorageVolPtr vol) { - VIR_DEBUG("vol=%p refs=%d", vol, vol ? vol->parent.u.s.refs : 0); + VIR_DEBUG("vol=%p", vol); virResetLastError(); diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 41b9cc1445..aeb7562018 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -85,8 +85,7 @@ virStreamNew(virConnectPtr conn, int virStreamRef(virStreamPtr stream) { - VIR_DEBUG("stream=%p refs=%d", stream, - stream ? stream->parent.u.s.refs : 0); + VIR_DEBUG("stream=%p", stream); virResetLastError(); diff --git a/src/libvirt_qemu_probes.d b/src/libvirt_qemu_probes.d index e4449a9922..5bcac7667b 100644 --- a/src/libvirt_qemu_probes.d +++ b/src/libvirt_qemu_probes.d @@ -4,10 +4,10 @@ provider libvirt { # binary: libvirtd # module: libvirt/connection-driver/libvirt_driver_qemu.so # Monitor lifecycle - probe qemu_monitor_new(void *mon, int refs, int fd); - probe qemu_monitor_ref(void *mon, int refs); - probe qemu_monitor_unref(void *mon, int refs); - probe qemu_monitor_close(void *monm, int refs); + probe qemu_monitor_new(void *mon, int fd); + probe qemu_monitor_ref(void *mon); + probe qemu_monitor_unref(void *mon); + probe qemu_monitor_close(void *monm); # High level monitor message processing probe qemu_monitor_send_msg(void *mon, const char *msg, int fd); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ff7d66eee5..94fae66708 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -714,9 +714,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, virObjectLock(mon); qemuMonitorRegister(mon); - PROBE(QEMU_MONITOR_NEW, - "mon=%p refs=%d fd=%d", - mon, mon->parent.parent.u.s.refs, mon->fd); + PROBE(QEMU_MONITOR_NEW, "mon=%p fd=%d", mon, mon->fd); virObjectUnlock(mon); return mon; @@ -865,8 +863,7 @@ qemuMonitorClose(qemuMonitorPtr mon) return; virObjectLock(mon); - PROBE(QEMU_MONITOR_CLOSE, - "mon=%p refs=%d", mon, mon->parent.parent.u.s.refs); + PROBE(QEMU_MONITOR_CLOSE, "mon=%p", mon); qemuMonitorSetDomainLogLocked(mon, NULL, NULL, NULL); -- 2.24.1
Memory allocated using g_object_new must never be released using VIR_FREE/g_free because g_object_new uses a special allocation strategy internally. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnettlscontext.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 02c17124a1..ced0cbdcd8 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -750,12 +750,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, return ctxt; error: + virObjectUnref(ctxt); if (isServer) gnutls_dh_params_deinit(ctxt->dhParams); - if (ctxt->x509cred) - gnutls_certificate_free_credentials(ctxt->x509cred); - VIR_FREE(ctxt->priority); - VIR_FREE(ctxt); return NULL; } -- 2.24.1
On 5/19/20 7:41 PM, Daniel P. Berrangé wrote:
Memory allocated using g_object_new must never be released using VIR_FREE/g_free because g_object_new uses a special allocation strategy internally.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnettlscontext.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 02c17124a1..ced0cbdcd8 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -750,12 +750,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, return ctxt;
error: + virObjectUnref(ctxt); if (isServer) gnutls_dh_params_deinit(ctxt->dhParams); - if (ctxt->x509cred) - gnutls_certificate_free_credentials(ctxt->x509cred); - VIR_FREE(ctxt->priority); - VIR_FREE(ctxt);
The unref call needs to go exactly here, where you remove these lines, because at the point we jump onto the error label, @ctxt has exactly one reference. And if you decrease it, the object is freed and the subsequent call to gnutls_whatever() would deref invalid pointer. Michal
To avoid bugs with mixing of g_object_(ref|unref) vs virObject(Ref|Unref), we want every virObject to be a GObject. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virobject.c | 141 +++++++++++++++++++++++++------------------ src/util/virobject.h | 25 +++----- 2 files changed, 90 insertions(+), 76 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 4060d7307b..3f0bcc38ea 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -39,6 +39,7 @@ static unsigned int magicCounter = 0xCAFE0000; struct _virClass { virClassPtr parent; + GType type; unsigned int magic; char *name; size_t objectSize; @@ -46,25 +47,28 @@ struct _virClass { virObjectDisposeCallback dispose; }; -#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)) +typedef struct _virObjectPrivate virObjectPrivate; +struct _virObjectPrivate { + virClassPtr klass; +}; + + +G_DEFINE_TYPE_WITH_PRIVATE(virObject, vir_object, G_TYPE_OBJECT) + +#define VIR_OBJECT_NOTVALID(obj) (!obj || !VIR_IS_OBJECT(obj)) #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (VIR_OBJECT_NOTVALID(obj)) { \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ - VIR_WARN("Object %p has a bad magic number %X", \ - obj, obj->u.s.magic); \ - } else { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + if (VIR_OBJECT_NOTVALID(obj)) \ VIR_WARN("Object %p (%s) is not a %s instance", \ - anyobj, obj->klass->name, #objclass); \ - } \ + anyobj, g_type_name_from_instance((void*)anyobj), #objclass); \ } while (0) -static virClassPtr virObjectClass; +static virClassPtr virObjectClassImpl; static virClassPtr virObjectLockableClass; static virClassPtr virObjectRWLockableClass; @@ -74,17 +78,17 @@ static void virObjectRWLockableDispose(void *anyobj); static int virObjectOnceInit(void) { - if (!(virObjectClass = virClassNew(NULL, - "virObject", - sizeof(virObject), - 0, - NULL))) + if (!(virObjectClassImpl = virClassNew(NULL, + "virObject", + sizeof(virObject), + 0, + NULL))) return -1; - if (!VIR_CLASS_NEW(virObjectLockable, virObjectClass)) + if (!VIR_CLASS_NEW(virObjectLockable, virObjectClassImpl)) return -1; - if (!VIR_CLASS_NEW(virObjectRWLockable, virObjectClass)) + if (!VIR_CLASS_NEW(virObjectRWLockable, virObjectClassImpl)) return -1; return 0; @@ -104,7 +108,7 @@ virClassForObject(void) if (virObjectInitialize() < 0) return NULL; - return virObjectClass; + return virObjectClassImpl; } @@ -138,6 +142,14 @@ virClassForObjectRWLockable(void) } +static void virClassDummyInit(void *klass G_GNUC_UNUSED) +{ +} + +static void virObjectDummyInit(void *obj G_GNUC_UNUSED) +{ +} + /** * virClassNew: * @parent: the parent class @@ -177,25 +189,26 @@ virClassNew(virClassPtr parent, return NULL; } - if (VIR_ALLOC(klass) < 0) - goto error; - + klass = g_new0(virClass, 1); klass->parent = parent; klass->magic = g_atomic_int_add(&magicCounter, 1); - if (klass->magic > 0xCAFEFFFF) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("too many object classes defined")); - goto error; - } klass->name = g_strdup(name); klass->objectSize = objectSize; + if (parent == NULL) { + klass->type = vir_object_get_type(); + } else { + klass->type = + g_type_register_static_simple(parent->type, + name, + sizeof(virObjectClass), + (GClassInitFunc)virClassDummyInit, + objectSize, + (GInstanceInitFunc)virObjectDummyInit, + 0); + } klass->dispose = dispose; return klass; - - error: - VIR_FREE(klass); - return NULL; } @@ -237,17 +250,13 @@ void * virObjectNew(virClassPtr klass) { virObjectPtr obj = NULL; + virObjectPrivate *priv; - if (VIR_ALLOC_VAR(obj, - char, - klass->objectSize - sizeof(virObject)) < 0) - return NULL; - - obj->u.s.magic = klass->magic; - obj->klass = klass; - g_atomic_int_set(&obj->u.s.refs, 1); + obj = g_object_new(klass->type, NULL); - PROBE(OBJECT_NEW, "obj=%p classname=%s", obj, obj->klass->name); + priv = vir_object_get_instance_private(obj); + priv->klass = klass; + PROBE(OBJECT_NEW, "obj=%p classname=%s", obj, priv->klass->name); return obj; } @@ -304,6 +313,33 @@ virObjectRWLockableNew(virClassPtr klass) return obj; } +static void vir_object_finalize(GObject *gobj) +{ + PROBE(OBJECT_DISPOSE, "obj=%p", gobj); + virObjectPtr obj = VIR_OBJECT(gobj); + virObjectPrivate *priv = vir_object_get_instance_private(obj); + + virClassPtr klass = priv->klass; + while (klass) { + if (klass->dispose) + klass->dispose(obj); + klass = klass->parent; + } + + G_OBJECT_CLASS(vir_object_parent_class)->finalize(gobj); +} + +static void vir_object_init(virObject *obj G_GNUC_UNUSED) +{ +} + + +static void vir_object_class_init(virObjectClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = vir_object_finalize; +} static void virObjectLockableDispose(void *anyobj) @@ -340,23 +376,8 @@ virObjectUnref(void *anyobj) if (VIR_OBJECT_NOTVALID(obj)) return; - bool lastRef = !!g_atomic_int_dec_and_test(&obj->u.s.refs); + g_object_unref(anyobj); PROBE(OBJECT_UNREF, "obj=%p", obj); - if (lastRef) { - PROBE(OBJECT_DISPOSE, "obj=%p", obj); - virClassPtr klass = obj->klass; - while (klass) { - if (klass->dispose) - klass->dispose(obj); - klass = klass->parent; - } - - /* Clear & poison object */ - memset(obj, 0, obj->klass->objectSize); - obj->u.s.magic = 0xDEADBEEF; - obj->klass = (void*)0xDEADBEEF; - VIR_FREE(obj); - } } @@ -376,7 +397,8 @@ virObjectRef(void *anyobj) if (VIR_OBJECT_NOTVALID(obj)) return NULL; - g_atomic_int_add(&obj->u.s.refs, 1); + + g_object_ref(obj); PROBE(OBJECT_REF, "obj=%p", obj); return anyobj; } @@ -539,10 +561,13 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; + virObjectPrivate *priv; + if (VIR_OBJECT_NOTVALID(obj)) return false; - return virClassIsDerivedFrom(obj->klass, klass); + priv = vir_object_get_instance_private(obj); + return virClassIsDerivedFrom(priv->klass, klass); } diff --git a/src/util/virobject.h b/src/util/virobject.h index cfedb19b13..18a9f098a6 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -24,6 +24,8 @@ #include "internal.h" #include "virthread.h" +#include <glib-object.h> + typedef struct _virClass virClass; typedef virClass *virClassPtr; @@ -38,22 +40,11 @@ typedef virObjectRWLockable *virObjectRWLockablePtr; typedef void (*virObjectDisposeCallback)(void *obj); -/* Most code should not play with the contents of this struct; however, - * the struct itself is public so that it can be embedded as the first - * field of a subclassed object. */ -struct _virObject { - /* Ensure correct alignment of this and all subclasses, even on - * platforms where 'long long' or function pointers have stricter - * requirements than 'void *'. */ - union { - long long dummy_align1; - void (*dummy_align2) (void); - struct { - unsigned int magic; - int refs; - } s; - } u; - virClassPtr klass; +#define VIR_TYPE_OBJECT vir_object_get_type() +G_DECLARE_DERIVABLE_TYPE(virObject, vir_object, VIR, OBJECT, GObject); + +struct _virObjectClass { + GObjectClass parent; }; struct _virObjectLockable { @@ -109,8 +100,6 @@ virObjectNew(virClassPtr klass) void virObjectUnref(void *obj); -G_DEFINE_AUTOPTR_CLEANUP_FUNC(virObject, virObjectUnref); - void * virObjectRef(void *obj); -- 2.24.1
On 5/19/20 7:41 PM, Daniel P. Berrangé wrote:
This series attempts to make the conversion from virObject to GObject less dangerous, by making the former inherit from the latter.
The way we setup inheritance with virObject means we have to make some moderately gross games with registering virClass instances as GObjectClass instances. Empirically it seems to work and pass the basic TCK test suite.
Ultimately virObject should be deleted entirely, so the gross bits shouldn't live too long.
Daniel P. Berrangé (6): qemu: stop checking virObjectUnref return value src: make virObjectUnref return void test: allocate numa cells separately from driver src: don't include ref count in debug messages / probes src: don't use VIR_FREE on an object allocation src: make virObject inherit from GObject
src/admin/libvirt-admin.c | 7 +- src/datatypes.c | 26 ++++ src/datatypes.h | 6 + src/interface/interface_backend_netcf.c | 7 +- src/libvirt-domain-checkpoint.c | 3 +- src/libvirt-domain-snapshot.c | 3 +- src/libvirt-domain.c | 2 +- src/libvirt-host.c | 2 +- src/libvirt-interface.c | 2 +- src/libvirt-network.c | 6 +- src/libvirt-nodedev.c | 2 +- src/libvirt-nwfilter.c | 6 +- src/libvirt-secret.c | 3 +- src/libvirt-storage.c | 4 +- src/libvirt-stream.c | 3 +- src/libvirt.c | 4 +- src/libvirt_qemu_probes.d | 8 +- src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_monitor.c | 21 +++- src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_process.c | 30 ++--- src/rpc/virnettlscontext.c | 5 +- src/test/test_driver.c | 15 ++- src/util/virfdstream.c | 6 +- src/util/virobject.c | 150 ++++++++++++++---------- src/util/virobject.h | 27 ++--- src/vbox/vbox_common.c | 10 +- 27 files changed, 210 insertions(+), 155 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Michal Privoznik