[libvirt] [PATCH v2 3/4] qemu: Don't duplicate errors when settings stats period

[Erik Skultety]

On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> In order not to leave old error messages set, this patch refactors the
> code so the error is reported only when acted upon.  The only such place
> already rewrites any error, so cleaning up all the error reporting in
> qemuMonitorSetMemoryStatsPeriod() is enough.
> 
> +/**
> + * qemuMonitorSetMemoryStatsPeriod:
> + *
> + * This function sets balloon stats update period.
> + *
> + * Returns 0 on success and -1 on error, but does *not* set an error.
> + */
>  int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
>                                      int period)
>  {
>      int ret = -1;
>      VIR_DEBUG("mon=%p period=%d", mon, period);
> 
> -    if (!mon) {
> -        virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                       _("monitor must not be NULL"));
> +    if (!mon)
>          return -1;
> -    }
> 
> -    if (!mon->json) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                       _("JSON monitor is required"));
> +    if (!mon->json)
> +        return -1;
> +
> +    if (period < 0)
>          return -1;
> -    }

Hmm. It is a nice idea, but I guess you might have forgotten to check
the actual return value in qemuProcessStart (there are actually 2
appearances in qemu_process.c) like we do in most cases.
I found a piece of code (see below) where we check this correctly (so
it's great you refactored this, otherwise reporting 2 identical messages
would be sort of confusing)

src/qemu/qemu_driver.c
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
    goto endjob;
if (r < 0) {
    virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                   _("unable to set balloon driver collection period"));
    goto endjob;

>      if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) {
>          ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
>                                                    period);
> +
> +        /*
> +         * Most of the calls to this function are supposed to be
> +         * non-fatal and the only one that should be fatal wants its
> +         * own error message.  More details for debugging will be in
> +         * the log file.
> +         */
> +        if (ret < 0)
> +            virResetLastError();
>      }
>      mon->ballooninit = true;
>      return ret;


Everything else looks fine to me, though I think that other monitor
calls should meet a similar fate sometime in the future.
Erik