[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]