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

Martin Kletzander mkletzan at redhat.com
Mon Mar 16 16:42:40 UTC 2015


On Mon, Mar 16, 2015 at 02:09:39PM +0100, Erik Skultety wrote:
>
>
>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)
>

This function is called from three places.  When starting a domain,
when attaching to a domain and from an API that requests change to
this particular value.  First two calls are intentionally non-fatal
and hence not acted upon, since such minor issue as setting the
statistics gathering period shouldn't make domains non-startable.

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


More information about the libvir-list mailing list