In near future we will want to not report error when connecting
fails. In order to achieve that we have to pass a boolean that
suppresses error messages.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virsh-domain.c |  2 +-
 tools/virsh.c        | 67 ++++++++++++++++++++++++++++++++--------------------
 tools/virsh.h        |  5 +++-
 tools/virt-admin.c   | 49 +++++++++++++++++++++-----------------
 tools/vsh.c          |  2 +-
 tools/vsh.h          |  3 ++-
 6 files changed, 76 insertions(+), 52 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 42d552637..cf612f73e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10931,7 +10931,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
         if (vshCommandOptStringReq(ctl, cmd, "desturi", &desturi) < 0)
             goto cleanup;
 
-        dconn = virshConnect(ctl, desturi, false);
+        dconn = virshConnect(ctl, desturi, false, false);
         if (!dconn)
             goto cleanup;
 
diff --git a/tools/virsh.c b/tools/virsh.c
index d0c135016..7d6dc2620 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -137,7 +137,7 @@ virshCatchDisconnect(virConnectPtr conn,
 /* Main Function which should be used for connecting.
  * This function properly handles keepalive settings. */
 virConnectPtr
-virshConnect(vshControl *ctl, const char *uri, bool readonly)
+virshConnect(vshControl *ctl, const char *uri, bool readonly, bool silent)
 {
     virConnectPtr c = NULL;
     int interval = 5; /* Default */
@@ -191,15 +191,19 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
     if (interval > 0 &&
         virConnectSetKeepAlive(c, interval, count) != 0) {
         if (keepalive_forced) {
-            vshError(ctl, "%s",
-                     _("Cannot setup keepalive on connection "
-                       "as requested, disconnecting"));
+            if (!silent) {
+                vshError(ctl, "%s",
+                         _("Cannot setup keepalive on connection "
+                           "as requested, disconnecting"));
+            }
             virConnectClose(c);
             c = NULL;
             goto cleanup;
         }
-        vshDebug(ctl, VSH_ERR_INFO, "%s",
-                 _("Failed to setup keepalive on connection\n"));
+        if (!silent) {
+            vshDebug(ctl, VSH_ERR_INFO, "%s",
+                     _("Failed to setup keepalive on connection\n"));
+        }
     }
 
  cleanup:
@@ -214,7 +218,11 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
  *
  */
 static int
-virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force)
+virshReconnect(vshControl *ctl,
+               const char *name,
+               bool readonly,
+               bool force,
+               bool silent)
 {
     bool connected = false;
     virshControlPtr priv = ctl->privData;
@@ -232,20 +240,25 @@ virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force)
 
         virConnectUnregisterCloseCallback(priv->conn, virshCatchDisconnect);
         ret = virConnectClose(priv->conn);
-        if (ret < 0)
-            vshError(ctl, "%s", _("Failed to disconnect from the hypervisor"));
-        else if (ret > 0)
-            vshError(ctl, "%s", _("One or more references were leaked after "
-                                  "disconnect from the hypervisor"));
+        if (!silent) {
+            if (ret < 0)
+                vshError(ctl, "%s", _("Failed to disconnect from the hypervisor"));
+            else if (ret > 0)
+                vshError(ctl, "%s", _("One or more references were leaked after "
+                                      "disconnect from the hypervisor"));
+        }
     }
 
-    priv->conn = virshConnect(ctl, name ? name : ctl->connname, readonly);
+    priv->conn = virshConnect(ctl, name ? name : ctl->connname,
+                              readonly, silent);
 
     if (!priv->conn) {
-        if (disconnected)
-            vshError(ctl, "%s", _("Failed to reconnect to the hypervisor"));
-        else
-            vshError(ctl, "%s", _("failed to connect to the hypervisor"));
+        if (!silent) {
+            if (disconnected)
+                vshError(ctl, "%s", _("Failed to reconnect to the hypervisor"));
+            else
+                vshError(ctl, "%s", _("Failed to connect to the hypervisor"));
+        }
         return -1;
     } else {
         if (name) {
@@ -255,8 +268,9 @@ virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force)
         }
         if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
                                             ctl, NULL) < 0)
-            vshError(ctl, "%s", _("Unable to register disconnect callback"));
-        if (connected && !force)
+            if (!silent)
+                vshError(ctl, "%s", _("Unable to register disconnect callback"));
+        if (connected && !force && !silent)
             vshError(ctl, "%s", _("Reconnected to the hypervisor"));
     }
     disconnected = 0;
@@ -304,7 +318,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
         return false;
 
-    if (virshReconnect(ctl, name, ro, true) < 0)
+    if (virshReconnect(ctl, name, ro, true, false) < 0)
         return false;
 
     return true;
@@ -316,11 +330,12 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
  */
 
 static bool
-virshConnectionUsability(vshControl *ctl, virConnectPtr conn)
+virshConnectionUsability(vshControl *ctl, virConnectPtr conn, bool silent)
 {
     if (!conn ||
         virConnectIsAlive(conn) == 0) {
-        vshError(ctl, "%s", _("no valid connection"));
+        if (!silent)
+            vshError(ctl, "%s", _("no valid connection"));
         return false;
     }
 
@@ -333,15 +348,15 @@ virshConnectionUsability(vshControl *ctl, virConnectPtr conn)
 }
 
 static void *
-virshConnectionHandler(vshControl *ctl)
+virshConnectionHandler(vshControl *ctl, bool silent)
 {
     virshControlPtr priv = ctl->privData;
 
     if ((!priv->conn || disconnected) &&
-        virshReconnect(ctl, NULL, false, false) < 0)
+        virshReconnect(ctl, NULL, false, false, silent) < 0)
         return NULL;
 
-    if (virshConnectionUsability(ctl, priv->conn))
+    if (virshConnectionUsability(ctl, priv->conn, silent))
         return priv->conn;
 
     return NULL;
@@ -391,7 +406,7 @@ virshInit(vshControl *ctl)
          * non-default connection, or might be 'help' which needs no
          * connection).
          */
-        if (virshReconnect(ctl, NULL, false, false) < 0) {
+        if (virshReconnect(ctl, NULL, false, false, false) < 0) {
             vshReportError(ctl);
             return false;
         }
diff --git a/tools/virsh.h b/tools/virsh.h
index b353b645a..e03b597ef 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -144,6 +144,9 @@ typedef enum {
     VIRSH_BYMAC  = (1 << 4),
 } virshLookupByFlags;
 
-virConnectPtr virshConnect(vshControl *ctl, const char *uri, bool readonly);
+virConnectPtr virshConnect(vshControl *ctl,
+                           const char *uri,
+                           bool readonly,
+                           bool silent);
 
 #endif /* VIRSH_H */
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index b8b33af19..5d7ef7988 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -154,24 +154,26 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn ATTRIBUTE_UNUSED,
 }
 
 static int
-vshAdmConnect(vshControl *ctl, unsigned int flags)
+vshAdmConnect(vshControl *ctl, unsigned int flags, bool silent)
 {
     vshAdmControlPtr priv = ctl->privData;
 
     priv->conn = virAdmConnectOpen(ctl->connname, flags);
 
     if (!priv->conn) {
-        if (priv->wantReconnect)
-            vshError(ctl, "%s", _("Failed to reconnect to the admin server"));
-        else
-            vshError(ctl, "%s", _("Failed to connect to the admin server"));
+        if (!silent) {
+            if (priv->wantReconnect)
+                vshError(ctl, "%s", _("Failed to reconnect to the admin server"));
+            else
+                vshError(ctl, "%s", _("Failed to connect to the admin server"));
+        }
         return -1;
     } else {
         if (virAdmConnectRegisterCloseCallback(priv->conn, vshAdmCatchDisconnect,
-                                               NULL, NULL) < 0)
+                                               NULL, NULL) < 0 && !silent)
             vshError(ctl, "%s", _("Unable to register disconnect callback"));
 
-        if (priv->wantReconnect)
+        if (priv->wantReconnect && !silent)
             vshPrint(ctl, "%s\n", _("Reconnected to the admin server"));
     }
 
@@ -179,7 +181,7 @@ vshAdmConnect(vshControl *ctl, unsigned int flags)
 }
 
 static int
-vshAdmDisconnect(vshControl *ctl)
+vshAdmDisconnect(vshControl *ctl, bool silent)
 {
     int ret = 0;
     vshAdmControlPtr priv = ctl->privData;
@@ -189,11 +191,13 @@ vshAdmDisconnect(vshControl *ctl)
 
     virAdmConnectUnregisterCloseCallback(priv->conn, vshAdmCatchDisconnect);
     ret = virAdmConnectClose(priv->conn);
-    if (ret < 0)
-        vshError(ctl, "%s", _("Failed to disconnect from the admin server"));
-    else if (ret > 0)
-        vshError(ctl, "%s", _("One or more references were leaked after "
-                              "disconnect from the hypervisor"));
+    if (!silent) {
+        if (ret < 0)
+            vshError(ctl, "%s", _("Failed to disconnect from the admin server"));
+        else if (ret > 0)
+            vshError(ctl, "%s", _("One or more references were leaked after "
+                                  "disconnect from the hypervisor"));
+    }
     priv->conn = NULL;
     return ret;
 }
@@ -205,14 +209,14 @@ vshAdmDisconnect(vshControl *ctl)
  *
  */
 static void
-vshAdmReconnect(vshControl *ctl)
+vshAdmReconnect(vshControl *ctl, bool silent)
 {
     vshAdmControlPtr priv = ctl->privData;
     if (priv->conn)
         priv->wantReconnect = true;
 
-    vshAdmDisconnect(ctl);
-    vshAdmConnect(ctl, 0);
+    vshAdmDisconnect(ctl, silent);
+    vshAdmConnect(ctl, 0, silent);
 
     priv->wantReconnect = false;
 }
@@ -350,7 +354,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
         ctl->connname = vshStrdup(ctl, name);
     }
 
-    vshAdmReconnect(ctl);
+    vshAdmReconnect(ctl, false);
     if (!connected && priv->conn)
         vshPrint(ctl, "%s\n", _("Connected to the admin server"));
 
@@ -1080,15 +1084,16 @@ cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd)
 }
 
 static void *
-vshAdmConnectionHandler(vshControl *ctl)
+vshAdmConnectionHandler(vshControl *ctl, bool silent)
 {
     vshAdmControlPtr priv = ctl->privData;
 
     if (!virAdmConnectIsAlive(priv->conn))
-        vshAdmReconnect(ctl);
+        vshAdmReconnect(ctl, silent);
 
     if (!virAdmConnectIsAlive(priv->conn)) {
-        vshError(ctl, "%s", _("no valid connection"));
+        if (!silent)
+            vshError(ctl, "%s", _("no valid connection"));
         return NULL;
     }
 
@@ -1122,7 +1127,7 @@ vshAdmInit(vshControl *ctl)
     ctl->eventLoopStarted = true;
 
     if (ctl->connname) {
-        vshAdmReconnect(ctl);
+        vshAdmReconnect(ctl, false);
         /* Connecting to a named connection must succeed, but we delay
          * connecting to the default connection until we need it
          * (since the first command might be 'connect' which allows a
@@ -1156,7 +1161,7 @@ vshAdmDeinit(vshControl *ctl)
     VIR_FREE(ctl->connname);
 
     if (priv->conn)
-        vshAdmDisconnect(ctl);
+        vshAdmDisconnect(ctl, false);
 
     virResetLastError();
 
diff --git a/tools/vsh.c b/tools/vsh.c
index 24ea45aa4..83c96e1a8 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1321,7 +1321,7 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
             GETTIMEOFDAY(&before);
 
         if ((cmd->def->flags & VSH_CMD_FLAG_NOCONNECT) ||
-            (hooks && hooks->connHandler && hooks->connHandler(ctl))) {
+            (hooks && hooks->connHandler && hooks->connHandler(ctl, false))) {
             ret = cmd->def->handler(ctl, cmd);
         } else {
             /* connection is not usable, return error */
diff --git a/tools/vsh.h b/tools/vsh.h
index 8f7df9ff8..c411c2ca4 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -232,7 +232,8 @@ struct _vshControl {
 };
 
 typedef void *
-(*vshConnectionHook)(vshControl *ctl);
+(*vshConnectionHook)(vshControl *ctl,
+                     bool silent);
 
 struct _vshClientHooks {
     vshConnectionHook connHandler;
-- 
2.13.6

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

On Tue, Nov 07, 2017 at 01:22:51PM +0100, Michal Privoznik wrote:
>In near future we will want to not report error when connecting
>fails. In order to achieve that we have to pass a boolean that
>suppresses error messages.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/virsh-domain.c |  2 +-
> tools/virsh.c        | 67 ++++++++++++++++++++++++++++++++--------------------
> tools/virsh.h        |  5 +++-
> tools/virt-admin.c   | 49 +++++++++++++++++++++-----------------
> tools/vsh.c          |  2 +-
> tools/vsh.h          |  3 ++-
> 6 files changed, 76 insertions(+), 52 deletions(-)
>
>diff --git a/tools/virsh.c b/tools/virsh.c
>index d0c135016..7d6dc2620 100644
>--- a/tools/virsh.c
>+++ b/tools/virsh.c
>@@ -191,15 +191,19 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
>     if (interval > 0 &&
>         virConnectSetKeepAlive(c, interval, count) != 0) {
>         if (keepalive_forced) {
>-            vshError(ctl, "%s",
>-                     _("Cannot setup keepalive on connection "
>-                       "as requested, disconnecting"));
>+            if (!silent) {
>+                vshError(ctl, "%s",
>+                         _("Cannot setup keepalive on connection "
>+                           "as requested, disconnecting"));
>+            }
>             virConnectClose(c);
>             c = NULL;
>             goto cleanup;
>         }
>-        vshDebug(ctl, VSH_ERR_INFO, "%s",
>-                 _("Failed to setup keepalive on connection\n"));
>+        if (!silent) {
>+            vshDebug(ctl, VSH_ERR_INFO, "%s",
>+                     _("Failed to setup keepalive on connection\n"));
>+        }

I guess debug messages probably need to be filtered too.  Actually
allocation errors should be covered as well.  And you missed some.  It's
fine as it is, since no auto-completion is perfect, I use lot of them
extensively and I must say I don't care if it outputs something when
some error happens.  The nice thing about auto-completion is that if it
does not work at all or works differently (outputs something it
"shouldn't") nothing happens.  There is no data loss, broken migrations
or whatever.

Anyway looking through all the things that you are modifying here and to
those that could still be modified, I think this could be approached a
bit better.  As I said it's kind of fine if we keep it like this for
now, but it could be even better.  Consider this:

 - we do not modify what messages are reported
 - we leave the function that manages log output to decide
 - when the completer is called we set it up so that there is no log
   output

Guess what, we have first two things in place and for the third one you
can just call vshCloseLogFile(ctl) and you're golden ;)

If you want to make it even better, you can already do that before you
get to any completer.  For example if you specify '-q' multiple times on
the command line it could switch to super quiet mode, e.g.:

diff --git i/tools/virsh.c w/tools/virsh.c
index f830331f6bbe..6b7eafeba0be 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -782,6 +782,8 @@ virshParseArgv(vshControl *ctl, int argc, char **argv)
             vshOpenLogFile(ctl);
             break;
         case 'q':
+            if (ctl->quiet)
+                vshCloseLogFile(ctl);
             ctl->quiet = true;
             break;
         case 't':
--

And just call virsh from the bash completion with '-qq'

And the same for virt-admin of course.  And maybe document it :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list