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

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.