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

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)

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