[libvirt] [PATCHv2 7/7] qemu: monitor: Refactor and fix monitor checking

[Peter Krempa]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150415/d467fa7a/attachment-0001.sig>