[libvirt] [PATCH] unlock the monitor when unwatching the monitor
Steps to reproduce this bug: # virsh qemu-monitor-command domain 'cpu_set 2 online' --hmp The domain has 2 cpus, and we try to set the third cpu online. The qemu crashes, and this command will hang. The reason is that the refs is not 1 when we unwatch the monitor. We lock the monitor, but we do not unlock it. So virCondWait() will be blocked. Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- src/qemu/qemu_monitor.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index da38096..dc08594 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -239,7 +239,8 @@ qemuMonitorUnwatch(void *monitor) qemuMonitorPtr mon = monitor; qemuMonitorLock(mon); - qemuMonitorUnref(mon); + if (qemuMonitorUnref(mon) > 0) + qemuMonitorUnlock(mon); } static int -- 1.7.1
On Wed, Mar 16, 2011 at 02:54:28PM +0800, Wen Congyang wrote:
Steps to reproduce this bug: # virsh qemu-monitor-command domain 'cpu_set 2 online' --hmp The domain has 2 cpus, and we try to set the third cpu online. The qemu crashes, and this command will hang.
The reason is that the refs is not 1 when we unwatch the monitor. We lock the monitor, but we do not unlock it. So virCondWait() will be blocked.
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
--- src/qemu/qemu_monitor.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index da38096..dc08594 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -239,7 +239,8 @@ qemuMonitorUnwatch(void *monitor) qemuMonitorPtr mon = monitor;
qemuMonitorLock(mon); - qemuMonitorUnref(mon); + if (qemuMonitorUnref(mon) > 0) + qemuMonitorUnlock(mon); }
ACK We should probably annotate qemuMonitorUnref() (and similar functions) with ATTRIBUTE_RETURN_CHECK too Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/17/2011 04:54 AM, Daniel P. Berrange wrote:
On Wed, Mar 16, 2011 at 02:54:28PM +0800, Wen Congyang wrote:
Steps to reproduce this bug: # virsh qemu-monitor-command domain 'cpu_set 2 online' --hmp The domain has 2 cpus, and we try to set the third cpu online. The qemu crashes, and this command will hang.
The reason is that the refs is not 1 when we unwatch the monitor. We lock the monitor, but we do not unlock it. So virCondWait() will be blocked.
qemuMonitorLock(mon); - qemuMonitorUnref(mon); + if (qemuMonitorUnref(mon) > 0) + qemuMonitorUnlock(mon); }
ACK
Pushed.
We should probably annotate qemuMonitorUnref() (and similar functions) with ATTRIBUTE_RETURN_CHECK too
I'm working on that now... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The next patch will change reference counting idioms; consolidating this pattern now makes the next patch smaller (touch only the new macro rather than every caller). * src/qemu/qemu_monitor.c (QEMU_MONITOR_CALLBACK): New helper. (qemuMonitorGetDiskSecret, qemuMonitorEmitShutdown) (qemuMonitorEmitReset, qemuMonitorEmitPowerdown) (qemuMonitorEmitStop, qemuMonitorEmitRTCChange) (qemuMonitorEmitWatchdog, qemuMonitorEmitIOError) (qemuMonitorEmitGraphics): Use it to reduce duplication. ---
We should probably annotate qemuMonitorUnref() (and similar functions) with ATTRIBUTE_RETURN_CHECK too
I'm working on that now...
And here we go; this exercise didn't find any more unsafe instances. src/qemu/qemu_monitor.c | 80 +++++++++++++---------------------------------- 1 files changed, 22 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dc08594..fd02ca0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -754,6 +754,15 @@ int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, return qemuMonitorTextCommandWithFd(mon, cmd, scm_fd, reply); } +/* Ensure proper locking around callbacks; assumes mon and ret are in + * scope. */ +#define QEMU_MONITOR_CALLBACK(callback, ...) \ + qemuMonitorRef(mon); \ + qemuMonitorUnlock(mon); \ + if (mon->cb && mon->cb->callback) \ + ret = (mon->cb->callback)(mon, __VA_ARGS__); \ + qemuMonitorLock(mon); \ + qemuMonitorUnref(mon) int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, virConnectPtr conn, @@ -765,12 +774,8 @@ int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, *secret = NULL; *secretLen = 0; - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->diskSecretLookup) - ret = mon->cb->diskSecretLookup(mon, conn, mon->vm, path, secret, secretLen); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(diskSecretLookup, conn, mon->vm, + path, secret, secretLen); return ret; } @@ -780,12 +785,7 @@ int qemuMonitorEmitShutdown(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainShutdown) - ret = mon->cb->domainShutdown(mon, mon->vm); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainShutdown, mon->vm); return ret; } @@ -795,12 +795,7 @@ int qemuMonitorEmitReset(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainReset) - ret = mon->cb->domainReset(mon, mon->vm); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainReset, mon->vm); return ret; } @@ -810,12 +805,7 @@ int qemuMonitorEmitPowerdown(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainPowerdown) - ret = mon->cb->domainPowerdown(mon, mon->vm); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainPowerdown, mon->vm); return ret; } @@ -825,12 +815,7 @@ int qemuMonitorEmitStop(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainStop) - ret = mon->cb->domainStop(mon, mon->vm); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainStop, mon->vm); return ret; } @@ -840,12 +825,7 @@ int qemuMonitorEmitRTCChange(qemuMonitorPtr mon, long long offset) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainRTCChange) - ret = mon->cb->domainRTCChange(mon, mon->vm, offset); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainRTCChange, mon->vm, offset); return ret; } @@ -855,12 +835,7 @@ int qemuMonitorEmitWatchdog(qemuMonitorPtr mon, int action) int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainWatchdog) - ret = mon->cb->domainWatchdog(mon, mon->vm, action); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainWatchdog, mon->vm, action); return ret; } @@ -873,12 +848,7 @@ int qemuMonitorEmitIOError(qemuMonitorPtr mon, int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainIOError) - ret = mon->cb->domainIOError(mon, mon->vm, diskAlias, action, reason); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainIOError, mon->vm, diskAlias, action, reason); return ret; } @@ -898,16 +868,10 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon, int ret = -1; VIR_DEBUG("mon=%p", mon); - qemuMonitorRef(mon); - qemuMonitorUnlock(mon); - if (mon->cb && mon->cb->domainGraphics) - ret = mon->cb->domainGraphics(mon, mon->vm, - phase, - localFamily, localNode, localService, - remoteFamily, remoteNode, remoteService, - authScheme, x509dname, saslUsername); - qemuMonitorLock(mon); - qemuMonitorUnref(mon); + QEMU_MONITOR_CALLBACK(domainGraphics, mon->vm, phase, + localFamily, localNode, localService, + remoteFamily, remoteNode, remoteService, + authScheme, x509dname, saslUsername); return ret; } -- 1.7.4
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; } -- 1.7.4
On 03/18/2011 01:37 PM, Eric Blake wrote:
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(-)
ACK.
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; }
On 03/24/2011 02:43 PM, Laine Stump wrote:
On 03/18/2011 01:37 PM, Eric Blake wrote:
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(-)
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/3/18 Eric Blake <eblake@redhat.com>:
The next patch will change reference counting idioms; consolidating this pattern now makes the next patch smaller (touch only the new macro rather than every caller).
* src/qemu/qemu_monitor.c (QEMU_MONITOR_CALLBACK): New helper. (qemuMonitorGetDiskSecret, qemuMonitorEmitShutdown) (qemuMonitorEmitReset, qemuMonitorEmitPowerdown) (qemuMonitorEmitStop, qemuMonitorEmitRTCChange) (qemuMonitorEmitWatchdog, qemuMonitorEmitIOError) (qemuMonitorEmitGraphics): Use it to reduce duplication. ---
We should probably annotate qemuMonitorUnref() (and similar functions) with ATTRIBUTE_RETURN_CHECK too
I'm working on that now...
And here we go; this exercise didn't find any more unsafe instances.
src/qemu/qemu_monitor.c | 80 +++++++++++++---------------------------------- 1 files changed, 22 insertions(+), 58 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dc08594..fd02ca0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -754,6 +754,15 @@ int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, return qemuMonitorTextCommandWithFd(mon, cmd, scm_fd, reply); }
+/* Ensure proper locking around callbacks; assumes mon and ret are in + * scope. */ +#define QEMU_MONITOR_CALLBACK(callback, ...) \ + qemuMonitorRef(mon); \ + qemuMonitorUnlock(mon); \ + if (mon->cb && mon->cb->callback) \ + ret = (mon->cb->callback)(mon, __VA_ARGS__); \ + qemuMonitorLock(mon); \ + qemuMonitorUnref(mon)
Shouldn't this be #define QEMU_MONITOR_CALLBACK(callback, ...) \ do { \ ... \ } while (0) just to ensure that doing things like if (...) QEMU_MONITOR_CALLBACK(domainWatchdog, mon->vm, action); is safe? Because this one will break with the current form of QEMU_MONITOR_CALLBACK. Another nit on readability. The macro hides the conditional assignment of ret, but turning it into ret = QEMU_MONITOR_CALLBACK(...) is not that simple. ACK, with that do {} while (0) problem addressed. Matthias
On 03/19/2011 08:06 AM, Matthias Bolte wrote:
2011/3/18 Eric Blake <eblake@redhat.com>:
The next patch will change reference counting idioms; consolidating this pattern now makes the next patch smaller (touch only the new macro rather than every caller).
* src/qemu/qemu_monitor.c (QEMU_MONITOR_CALLBACK): New helper. (qemuMonitorGetDiskSecret, qemuMonitorEmitShutdown) (qemuMonitorEmitReset, qemuMonitorEmitPowerdown) (qemuMonitorEmitStop, qemuMonitorEmitRTCChange) (qemuMonitorEmitWatchdog, qemuMonitorEmitIOError) (qemuMonitorEmitGraphics): Use it to reduce duplication. ---
+/* Ensure proper locking around callbacks; assumes mon and ret are in + * scope. */ +#define QEMU_MONITOR_CALLBACK(callback, ...) \ + qemuMonitorRef(mon); \ + qemuMonitorUnlock(mon); \ + if (mon->cb && mon->cb->callback) \ + ret = (mon->cb->callback)(mon, __VA_ARGS__); \ + qemuMonitorLock(mon); \ + qemuMonitorUnref(mon)
Shouldn't this be
#define QEMU_MONITOR_CALLBACK(callback, ...) \ do { \ ... \ } while (0)
just to ensure that doing things like
if (...) QEMU_MONITOR_CALLBACK(domainWatchdog, mon->vm, action);
is safe? Because this one will break with the current form of QEMU_MONITOR_CALLBACK.
It would break, but none of the current callers used this macro from inside a conditional, so that's theoretical for now. Still, I agree that preventative programming can be worthwhile if it's not too tough.
Another nit on readability. The macro hides the conditional assignment of ret, but turning it into ret = QEMU_MONITOR_CALLBACK(...) is not that simple.
Yeah, so I decided to make mon and ret explicit parameters. Still not quite a true function (the macro uses a field name of the callback function, which is not your typical l- or rvalue of a real function call), but at least not as magic.
ACK, with that do {} while (0) problem addressed.
Here's what I squashed in, before pushing. Anyone want to review 2/2 of this series? diff --git i/src/qemu/qemu_monitor.c w/src/qemu/qemu_monitor.c index 5c409af..0c740ab 100644 --- i/src/qemu/qemu_monitor.c +++ w/src/qemu/qemu_monitor.c @@ -755,15 +755,16 @@ int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, return qemuMonitorTextCommandWithFd(mon, cmd, scm_fd, reply); } -/* Ensure proper locking around callbacks; assumes mon and ret are in - * scope. */ -#define QEMU_MONITOR_CALLBACK(callback, ...) \ - qemuMonitorRef(mon); \ - qemuMonitorUnlock(mon); \ - if (mon->cb && mon->cb->callback) \ - ret = (mon->cb->callback)(mon, __VA_ARGS__); \ - qemuMonitorLock(mon); \ - qemuMonitorUnref(mon) +/* Ensure proper locking around callbacks. */ +#define QEMU_MONITOR_CALLBACK(mon, ret, callback, ...) \ + do { \ + qemuMonitorRef(mon); \ + qemuMonitorUnlock(mon); \ + if ((mon)->cb && (mon)->cb->callback) \ + (ret) = ((mon)->cb->callback)(mon, __VA_ARGS__); \ + qemuMonitorLock(mon); \ + qemuMonitorUnref(mon); \ + } while (0) int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, virConnectPtr conn, @@ -775,7 +776,7 @@ int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, *secret = NULL; *secretLen = 0; - QEMU_MONITOR_CALLBACK(diskSecretLookup, conn, mon->vm, + QEMU_MONITOR_CALLBACK(mon, ret, diskSecretLookup, conn, mon->vm, path, secret, secretLen); return ret; } @@ -786,7 +787,7 @@ int qemuMonitorEmitShutdown(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainShutdown, mon->vm); + QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm); return ret; } @@ -796,7 +797,7 @@ int qemuMonitorEmitReset(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainReset, mon->vm); + QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm); return ret; } @@ -806,7 +807,7 @@ int qemuMonitorEmitPowerdown(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainPowerdown, mon->vm); + QEMU_MONITOR_CALLBACK(mon, ret, domainPowerdown, mon->vm); return ret; } @@ -816,7 +817,7 @@ int qemuMonitorEmitStop(qemuMonitorPtr mon) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainStop, mon->vm); + QEMU_MONITOR_CALLBACK(mon, ret, domainStop, mon->vm); return ret; } @@ -826,7 +827,7 @@ int qemuMonitorEmitRTCChange(qemuMonitorPtr mon, long long offset) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainRTCChange, mon->vm, offset); + QEMU_MONITOR_CALLBACK(mon, ret, domainRTCChange, mon->vm, offset); return ret; } @@ -836,7 +837,7 @@ int qemuMonitorEmitWatchdog(qemuMonitorPtr mon, int action) int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainWatchdog, mon->vm, action); + QEMU_MONITOR_CALLBACK(mon, ret, domainWatchdog, mon->vm, action); return ret; } @@ -849,7 +850,8 @@ int qemuMonitorEmitIOError(qemuMonitorPtr mon, int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainIOError, mon->vm, diskAlias, action, reason); + QEMU_MONITOR_CALLBACK(mon, ret, domainIOError, mon->vm, + diskAlias, action, reason); return ret; } @@ -869,7 +871,7 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon, int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(domainGraphics, mon->vm, phase, + QEMU_MONITOR_CALLBACK(mon, ret, domainGraphics, mon->vm, phase, localFamily, localNode, localService, remoteFamily, remoteNode, remoteService, authScheme, x509dname, saslUsername); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Matthias Bolte -
Wen Congyang