Re: [libvirt] [PATCH 2/2] build: enforce reference count checking

Add the compiler attribute to ensure we don't introduce any more ref bugs like were just patched in commit 9741f34, then explicitly mark the remaining places in code that are safe. * src/qemu/qemu_monitor.h (qemuMonitorUnref): Mark ATTRIBUTE_RETURN_CHECK. * src/conf/domain_conf.h (virDomainObjUnref): Likewise. * src/conf/domain_conf.c (virDomainObjParseXML) (virDomainLoadStatus): Fix offenders. * src/openvz/openvz_conf.c (openvzLoadDomains): Likewise. * src/vmware/vmware_conf.c (vmwareLoadDomains): Likewise. * src/qemu/qemu_domain.c (qemuDomainObjBeginJob) (qemuDomainObjBeginJobWithDriver) (qemuDomainObjExitRemoteWithDriver): Likewise. * src/qemu/qemu_monitor.c (QEMU_MONITOR_CALLBACK): Likewise. Suggested by Daniel P. Berrange. --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 2 +- src/openvz/openvz_conf.c | 6 ++++-- src/qemu/qemu_domain.c | 11 ++++++++--- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_process.c | 3 ++- src/vmware/vmware_conf.c | 7 +++++-- 8 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1b02c25..b681dc3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6015,7 +6015,8 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, return obj; error: - virDomainObjUnref(obj); + /* obj was never shared, so unref should return 0 */ + ignore_value(virDomainObjUnref(obj)); return NULL; } @@ -8220,8 +8221,9 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, return obj; error: + /* obj was never shared, so unref should return 0 */ if (obj) - virDomainObjUnref(obj); + ignore_value(virDomainObjUnref(obj)); VIR_FREE(statusFile); return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9f595d6..1e8223f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1206,7 +1206,7 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, void virDomainDefFree(virDomainDefPtr vm); void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ -int virDomainObjUnref(virDomainObjPtr vm); +int virDomainObjUnref(virDomainObjPtr vm) ATTRIBUTE_RETURN_CHECK; /* live == true means def describes an active domain (being migrated or * restored) as opposed to a new persistent configuration of the domain */ diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 0eb5ab3..b9e1e7e 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -1,7 +1,7 @@ /* * openvz_conf.c: config functions for managing OpenVZ VEs * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010, 2011 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * Copyright (C) 2007 Anoop Joe Cyriac @@ -52,6 +52,7 @@ #include "nodeinfo.h" #include "files.h" #include "command.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -543,8 +544,9 @@ int openvzLoadDomains(struct openvz_driver *driver) { cleanup: virCommandFree(cmd); VIR_FREE(outbuf); + /* dom hasn't been shared yet, so unref should return 0 */ if (dom) - virDomainObjUnref(dom); + ignore_value(virDomainObjUnref(dom)); return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cc137d2..c2a1f9a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -31,6 +31,7 @@ #include "c-ctype.h" #include "event.h" #include "cpu/cpu.h" +#include "ignore-value.h" #include<sys/time.h> @@ -460,7 +461,8 @@ int qemuDomainObjBeginJob(virDomainObjPtr obj) while (priv->jobActive) { if (virCondWaitUntil(&priv->jobCond,&obj->lock, then)< 0) { - virDomainObjUnref(obj); + /* Safe to ignore value since ref count was incremented above */ + ignore_value(virDomainObjUnref(obj)); if (errno == ETIMEDOUT) qemuReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", _("cannot acquire state change lock")); @@ -504,7 +506,8 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, while (priv->jobActive) { if (virCondWaitUntil(&priv->jobCond,&obj->lock, then)< 0) { - virDomainObjUnref(obj); + /* Safe to ignore value since ref count was incremented above */ + ignore_value(virDomainObjUnref(obj)); if (errno == ETIMEDOUT) qemuReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", _("cannot acquire state change lock")); @@ -650,7 +653,9 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, { qemuDriverLock(driver); virDomainObjLock(obj); - virDomainObjUnref(obj); + /* Safe to ignore value, since we incremented ref in + * qemuDomainObjEnterRemoteWithDriver */ + ignore_value(virDomainObjUnref(obj)); } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fd02ca0..0ce1075 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -762,7 +762,7 @@ int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, if (mon->cb&& mon->cb->callback) \ ret = (mon->cb->callback)(mon, __VA_ARGS__); \ qemuMonitorLock(mon); \ - qemuMonitorUnref(mon) + ignore_value(qemuMonitorUnref(mon)) int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, virConnectPtr conn, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7bea083..f9d3233 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -131,7 +131,7 @@ void qemuMonitorLock(qemuMonitorPtr mon); void qemuMonitorUnlock(qemuMonitorPtr mon); int qemuMonitorRef(qemuMonitorPtr mon); -int qemuMonitorUnref(qemuMonitorPtr mon); +int qemuMonitorUnref(qemuMonitorPtr mon) ATTRIBUTE_RETURN_CHECK; /* These APIs are for use by the internal Text/JSON monitor impl code only */ int qemuMonitorSend(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 740684a..ac504f5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -643,8 +643,9 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm) priv->monJSON, &monitorCallbacks); + /* Safe to ignore value since ref count was incremented above */ if (priv->mon == NULL) - virDomainObjUnref(vm); + ignore_value(virDomainObjUnref(vm)); if (virSecurityManagerClearSocketLabel(driver->securityManager, vm)< 0) { VIR_ERROR(_("Failed to clear security context for monitor for %s"), diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index c3f53ea..6339248 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -1,5 +1,7 @@ /*---------------------------------------------------------------------------*/ -/* Copyright 2010, diateam (www.diateam.net) +/* + * Copyright (C) 2011 Red Hat, Inc. + * Copyright 2010, diateam (www.diateam.net) * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -201,8 +203,9 @@ cleanup: VIR_FREE(directoryName); VIR_FREE(fileName); VIR_FREE(vmx); + /* any non-NULL vm here has not been shared, so unref will return 0 */ if (vm) - virDomainObjUnref(vm); + ignore_value(virDomainObjUnref(vm)); return ret; }