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

Erik Skultety eskultet at redhat.com
Tue Mar 17 08:55:38 UTC 2015



On 03/16/2015 05:42 PM, Martin Kletzander wrote:
> 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.
> 
In that case, it's an ACK.
Erik




More information about the libvir-list mailing list