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