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

[Jiri Denemark]

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.

>  {
>      VIR_DEBUG("mon=%p major=%p minor=%p micro=%p package=%p",
>                mon, major, minor, micro, package);
> 
> -    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 qemuMonitorJSONGetVersion(mon, major, minor, micro, package);
>  }
...

Jirka