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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list