On 03/19/2011 08:06 AM, Matthias Bolte wrote:
> 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.
>> ---
>>
>>
>> +/* 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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list