On Wed, Apr 15, 2015 at 10:24:10 +0200, Jiri Denemark wrote:
> On Tue, Apr 14, 2015 at 18:05:11 +0200, Peter Krempa wrote:
> > Among all the monitor APIs some where checking if mon is NULL and some
> > were not. Since it's possible to have mon equal to NULL in case a second
> > call is attempted once entered the monitor. This requires that every
> > single API checks for the monitor.
> >
> > This patch adds a macro that helps checking the state of the monitor and
> > either refactors existing checking code to use the macro or adds it in
> > case it was missing.
> > ---
> > src/qemu/qemu_monitor.c | 918 +++++++++---------------------------------------
> > src/qemu/qemu_monitor.h | 18 +-
> > 2 files changed, 174 insertions(+), 762 deletions(-)
> >
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index d5bdfb8..e5630f1 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -98,6 +98,32 @@ struct _qemuMonitor {
> > int logfd;
> > };
> >
> > +/**
> > + * QEMU_CHECK_MONITOR_FULL:
> > + * @MON: monitor pointer variable to check, evaluated multiple times, no parentheses
> > + * @FORCE_JSON: force JSON monitor, true or false
> > + * @EXIT: statement that is used to exit the function
> > + *
> > + * This macro checks that the monitor is valid for given operation and exits
> > + * the function if not. The macro also adds a debug statement regarding the
> > + * monitor.
> > + */
> > +#define QEMU_CHECK_MONITOR_FULL(MON, FORCE_JSON, EXIT) \
>
> The uppercase parameters look ugly, I think we use lowercase everywhere.
>
> > + if (!MON) { \
> > + virReportError(VIR_ERR_INVALID_ARG, "%s", \
> > + _("Monitor must not be NULL")); \
>
> s/Monitor/monitor/
>
> > + EXIT; \
> > + } \
> > + VIR_DEBUG("Mon:%p vm:%p json:%d fd:%d", MON, MON->vm, MON->json, MON->fd); \
>
> s/Mon/mon/
>
> > + if (FORCE_JSON && !MON->json) { \
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", \
> > + _("JSON monitor is required")); \
> > + EXIT; \
> > + }
> > +
> > +#define QEMU_CHECK_MONITOR(MON) QEMU_CHECK_MONITOR_FULL(MON, false, return -1)
> > +#define QEMU_CHECK_MONITOR_JSON(MON) QEMU_CHECK_MONITOR_FULL(MON, true, return -1)
>
> I think it would be nice to add a few more macros so that the FULL
> variant does not have to be used anywhere. Especially having to use
> true/false instead of QEMU_CHECK_MONITOR{_JSON,} is less obvious :-)
Indeed. I was thinking about those but ended up not finishing that idea.
>
> > +
> > static virClassPtr qemuMonitorClass;
> > static void qemuMonitorDispose(void *obj);
> >
> ...
> > @@ -1661,11 +1651,7 @@ qemuMonitorSetLink(qemuMonitorPtr mon,
> > {
> > VIR_DEBUG("mon=%p, name=%s, state=%u", mon, name, state);
>
> Should we remove "mon=%p" from this (and other similar) VIR_DEBUG since
> it will be logged by QEMU_CHECK_MONITOR?
I didn't want to touch the existing debug macros but I might as well go
through the code and finish that up.
>
> >
> > - if (!mon) {
> > - virReportError(VIR_ERR_INVALID_ARG, "%s",
> > - _("monitor must not be NULL"));
> > - return -1;
> > - }
> > + QEMU_CHECK_MONITOR(mon);
> >
> > if (mon->json)
> > return qemuMonitorJSONSetLink(mon, name, state);
> ...
> > @@ -3763,44 +3423,23 @@ qemuMonitorOpenGraphics(qemuMonitorPtr mon,
> > int
> > qemuMonitorSystemWakeup(qemuMonitorPtr mon)
> > {
> > - VIR_DEBUG("mon=%p", mon);
> > -
> > - if (!mon) {
> > - virReportError(VIR_ERR_INVALID_ARG, "%s",
> > - _("monitor must not be NULL"));
> > - return -1;
> > - }
> > -
> > - if (!mon->json) {
> > - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > - _("JSON monitor is required"));
> > - return -1;
> > - }
> > + QEMU_CHECK_MONITOR_JSON(mon);
> >
> > return qemuMonitorJSONSystemWakeup(mon);
> > }
> >
> >
> > -int qemuMonitorGetVersion(qemuMonitorPtr mon,
> > - int *major,
> > - int *minor,
> > - int *micro,
> > - char **package)
> > +int
> > +qemuMonitorGetVersion(qemuMonitorPtr mon,
> > + int *major,
> > + int *minor,
> > + int *micro,
> > + char **package)
>
> This change should go to the first patch in this series.
I've moved this to the first patch and pushed 1-6.
Thanks
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list