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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: pgpqOx9W2ROS3.pgp
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]