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

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 :-)

> + > 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?

> > - 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.