2011/3/18 Eric Blake <eblake@xxxxxxxxxx>:
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list