[PATCH 4/8] Remove all uses of virshReportError except "vshError".

[Chris Lalancette <clalance@xxxxxxxxxx>]

With the current virsh code, error reporting is a bit fragile
because the reporting is decoupled from the place where the
error occurred.  There are two places where this can be a problem:

1)  In utility functions that don't properly dispatch errors
like the main API functions do.  In this case, the error from
the utility function initially gets stored, but then may get
blown away during a cleanup function.

2)  If a cleanup function has an additional error.  In this case,
what will happen is that we will report the error from the cleanup
function, not the original error, which may make the problem
harder to diagnose.

What this patch does is to report errors exactly at the point
that they occurred, in vshError().  That way the individual
cmd* functions can do whatever cleanup is necessary without
fear of blowing away the appropriate error.

With this in place, it's now the responsibility of the
individual cmd* functions to use vshError when an error
occurs.  However, this should make error reporting more consistent.

Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>
---
 tools/virsh.c |   22 +++++-----------------
 1 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index c1451d8..015d038 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -377,7 +377,7 @@ virshErrorHandler(void *unused ATTRIBUTE_UNUSED, virErrorPtr error)
  * and it's IMHO a bug that libvirt does that sometimes.
  */
 static void
-virshReportError(vshControl *ctl)
+virshReportError(void)
 {
     if (last_error == NULL) {
         /* Calling directly into libvirt util functions won't trigger the
@@ -391,11 +391,11 @@ virshReportError(vshControl *ctl)
     }
 
     if (last_error->code == VIR_ERR_OK) {
-        vshError(ctl, "%s", _("unknown error"));
+        fprintf(stderr, "error: %s\n", _("unknown error"));
         goto out;
     }
 
-    vshError(ctl, "%s", last_error->message);
+    fprintf(stderr, "error: %s\n", last_error->message);
 
 out:
     virFreeError(last_error);
@@ -5911,7 +5911,6 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd)
     }
 
     if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-        virshReportError(ctl);
         virStoragePoolFree(pool);
         return FALSE;
     }
@@ -5973,7 +5972,6 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd)
         goto cleanup;
 
     if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-        virshReportError(ctl);
         goto cleanup;
     }
 
@@ -7470,7 +7468,6 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
     }
 
     if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-        virshReportError(ctl);
         virDomainFree(dom);
         return FALSE;
     }
@@ -7538,7 +7535,6 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
     }
 
     if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-        virshReportError(ctl);
         virDomainFree(dom);
         return FALSE;
     }
@@ -7606,7 +7602,6 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd)
     }
 
     if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-        virshReportError(ctl);
         virDomainFree(dom);
         return FALSE;
     }
@@ -8727,11 +8722,6 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
         buffer = strdup("<domainsnapshot/>");
     else {
         if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
-            /* we have to report the error here because during cleanup
-             * we'll run through virDomainFree(), which loses the
-             * last error
-             */
-            virshReportError(ctl);
             goto cleanup;
         }
     }
@@ -9910,9 +9900,6 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
         if (enable_timing)
             GETTIMEOFDAY(&after);
 
-        if (ret == FALSE)
-            virshReportError(ctl);
-
         /* try to automatically catch disconnections */
         if ((ret == FALSE) &&
             ((disconnected != 0) ||
@@ -10268,6 +10255,8 @@ vshError(vshControl *ctl, const char *format, ...)
     va_end(ap);
 
     fputc('\n', stderr);
+
+    virshReportError();
 }
 
 static void *
@@ -10348,7 +10337,6 @@ vshInit(vshControl *ctl)
      * such as "help".
      */
     if (!ctl->conn) {
-        virshReportError(ctl);
         vshError(ctl, "%s", _("failed to connect to the hypervisor"));
         return FALSE;
     }
-- 
1.7.1.1

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