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
pgpcztVq1JrgX.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
