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