[PATCH v2] tools: Fix connect command

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

 



The man page says: "(Re)-Connect to the hypervisor. When the shell is
first started, this is automatically run with the URI parameter
requested by the "-c" option on the command line."  However, if you run:

  virsh -c 'test://default' 'connect; uri'

the output will not be 'test://default'.  That's because the 'connect'
command does not care about any virsh-only related settings and if it is
run without parameters, it connects with @uri == NULL.  Not only that
doesn't comply to what the man page describes, but it also doesn't make
sense.  It also means you aren't able to reconnect to whatever you are
connected currently.

So let's fix that in both virsh and virt-admin add a test case for it.

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
v2:
 - rebased on top of Cole's changes

v1:
 - https://www.redhat.com/archives/libvir-list/2016-April/msg01462.html

 tests/test-lib.sh         |  6 ++++++
 tests/virsh-uriprecedence | 54 +++++++++++++++++++++++++++++++++++------------
 tools/virsh.c             | 48 ++++++++++-------------------------------
 tools/virt-admin.c        |  6 ++++--
 4 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index 8e0ce83e118c..49e8d2209572 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -232,6 +232,12 @@ if test -n "$VIR_TEST_DEBUG" || test -n "$VIR_TEST_VERBOSE" ; then
   verbose=1
 fi

+debug() { :; }
+
+if test "$VIR_TEST_DEBUG" = "2"; then
+  debug() { echo "$@"; }
+fi
+
 # This is a stub function that is run upon trap (upon regular exit and
 # interrupt).  Override it with a per-test function, e.g., to unmount
 # a partition, or to undo any other global state changes.
diff --git a/tests/virsh-uriprecedence b/tests/virsh-uriprecedence
index f9e325658900..4bcce3aeb08f 100755
--- a/tests/virsh-uriprecedence
+++ b/tests/virsh-uriprecedence
@@ -8,7 +8,8 @@
 test_intro "virsh-uriprecedence"

 virsh_bin="$abs_top_builddir/tools/virsh"
-counter=1
+virsh_cmd="$virsh_bin"
+counter=0
 ret=0

 cleanup_() { rm -rf "$tmphome"; }
@@ -23,16 +24,44 @@ mkdir -p "$XDG_CONFIG_HOME/libvirt" "$XDG_CONFIG_HOME/virsh"
 mkdir -p "$XDG_CACHE_HOME/libvirt" "$XDG_CACHE_HOME/virsh"
 mkdir -p "$XDG_RUNTIME_HOME/libvirt" "$XDG_RUNTIME_HOME/virsh"

-# Main function checking for the proper uri being returned
+is_uri_good()
+{
+    echo "$1" | grep -q -F "$good_uri"
+}
+
+test_uri_internal()
+{
+    test_name=$1
+    test_cmd="$virsh_cmd \"$2\""
+    result=0
+
+    debug "Running '$test_cmd'"
+    out="$($virsh_cmd "$2")"
+
+    if ! is_uri_good "$out"; then
+        debug "Invalid output: '$out'"
+        result=1
+        ret=1
+    fi
+
+    counter="$((counter+1))"
+    test_result "$counter" "$1" "$result"
+}
+
+test_uri_connect()
+{
+    test_uri_internal "$1" "connect; uri"
+}
+
+test_uri_noconnect()
+{
+    test_uri_internal "$1" "uri"
+}
+
 test_uri()
 {
-  result=0
-  if [ "$($virsh_bin uri)" != "$good_uri" ]; then
-      result=1
-      ret=1
-  fi
-  test_result "$counter" "$1" "$result"
-  counter="$((counter+1))"
+    test_uri_connect "$1"
+    test_uri_noconnect "$1"
 }

 # Precedence is the following (lowest priority first):
@@ -57,6 +86,7 @@ good_uri="test:///default?good_uri"

 printf "uri_default=\"%s\"\n" "$good_uri" >"$XDG_CONFIG_HOME/libvirt/libvirt.conf"
 if uid_is_privileged_; then
+    counter="$((counter+1))"
     test_skip_case "$counter" "User config file" "must not be run as root"
 else
     test_uri "User config file"
@@ -71,10 +101,8 @@ export VIRSH_DEFAULT_CONNECT_URI="$good_uri"
 test_uri "VIRSH_DEFAULT_CONNECT_URI"

 export VIRSH_DEFAULT_CONNECT_URI="$bad_uri"
-virsh_bin="$virsh_bin --connect $good_uri"
+virsh_cmd="$virsh_bin --connect $good_uri"
 test_uri "Parameter"

-# test_uri() increases $counter even for the last test, so we must
-# decrement it
-test_final "$((counter-1))" "$ret"
+test_final "$counter" "$ret"
 (exit "$ret"); exit "$ret"
diff --git a/tools/virsh.c b/tools/virsh.c
index af072510870d..00330db23d0a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -211,10 +211,11 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
  *
  */
 static void
-virshReconnect(vshControl *ctl)
+virshReconnect(vshControl *ctl, const char *name, bool readonly)
 {
     bool connected = false;
     virshControlPtr priv = ctl->privData;
+    bool ro = name ? readonly : priv->readonly;

     if (priv->conn) {
         int ret;
@@ -229,7 +230,7 @@ virshReconnect(vshControl *ctl)
                                   "disconnect from the hypervisor"));
     }

-    priv->conn = virshConnect(ctl, ctl->connname, priv->readonly);
+    priv->conn = virshConnect(ctl, name ? name : ctl->connname, ro);

     if (!priv->conn) {
         if (disconnected)
@@ -237,6 +238,11 @@ virshReconnect(vshControl *ctl)
         else
             vshError(ctl, "%s", _("failed to connect to the hypervisor"));
     } else {
+        if (name) {
+            VIR_FREE(ctl->connname);
+            ctl->connname = vshStrdup(ctl, name);
+            priv->readonly = readonly;
+        }
         if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
                                             ctl, NULL) < 0)
             vshError(ctl, "%s", _("Unable to register disconnect callback"));
@@ -291,43 +297,11 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
 {
     bool ro = vshCommandOptBool(cmd, "readonly");
     const char *name = NULL;
-    virshControlPtr priv = ctl->privData;
-    virConnectPtr conn;

     if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
         return false;

-    conn = virshConnect(ctl, name, ro);
-
-    if (!conn) {
-        vshError(ctl, "%s", _("Failed to connect to the hypervisor"));
-        return false;
-    }
-
-    if (priv->conn) {
-        int ret;
-
-        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"));
-    }
-    priv->conn = conn;
-
-    VIR_FREE(ctl->connname);
-    ctl->connname = vshStrdup(ctl, name);
-
-    priv->useGetInfo = false;
-    priv->useSnapshotOld = false;
-    priv->blockJobNoBytes = false;
-    priv->readonly = ro;
-
-    if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
-                                        ctl, NULL) < 0)
-        vshError(ctl, "%s", _("Unable to register disconnect callback"));
+    virshReconnect(ctl, name, ro);

     return true;
 }
@@ -360,7 +334,7 @@ virshConnectionHandler(vshControl *ctl)
     virshControlPtr priv = ctl->privData;

     if (!priv->conn || disconnected)
-        virshReconnect(ctl);
+        virshReconnect(ctl, NULL, false);

     if (virshConnectionUsability(ctl, priv->conn))
         return priv->conn;
@@ -431,7 +405,7 @@ virshInit(vshControl *ctl)
         return false;

     if (ctl->connname) {
-        virshReconnect(ctl);
+        virshReconnect(ctl, NULL, 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
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 22160ad929d0..4275aa37a1bf 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -291,8 +291,10 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
         return false;

-    VIR_FREE(ctl->connname);
-    ctl->connname = vshStrdup(ctl, name);
+    if (name) {
+        VIR_FREE(ctl->connname);
+        ctl->connname = vshStrdup(ctl, name);
+    }

     vshAdmReconnect(ctl);
     if (!connected)
--
2.8.1

--
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]