Re: [libvirt] [PATCH 03/11] vsh: Add @silent to vshConnectionHook

[Martin Kletzander]

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 
---
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 :)


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 03/11] vsh: Add @silent to vshConnectionHook

[Michal Privoznik]

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 
---
 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", ) < 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",