[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