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