[libvirt] [PATCH 1/2] qemu: simplify monitor callbacks

[Matthias Bolte]

2011/3/18 Eric Blake <eblake at 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