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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list