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