Improve error reporting for virConnectOpen URIs

ChangeLog

@@ -1,3 +1,17 @@
+Thu Jun 12 13:06:42 BST 2009 Daniel P. Berrange <berrange@redhat.com>
+
+	Improve error reporting for virConnecOpen URIs
+	* src/lxc_driver.c, src/openvz_driver.c, src/qemu_driver.c,
+	src/uml_driver.c, src/xen_unified.c: Always return ACCEPT
+	or ERROR for URIs without hostname set, but with the driver's
+	matching URI scheme. ie never decline a correct URI
+	* src/xend_internal.c: Default port to 8000 if not given
+	in the http:// URI.
+	* src/remote_internal.c: Accept all URIs not handled by an
+	earlier driver.
+	* src/virterror.c: Improve error message text for
+	VIR_ERR_NO_CONNECT code
+
 Thu Jun 12 12:26:42 BST 2009 Daniel P. Berrange <berrange@redhat.com>
 
 	Fix re-detection of transient VMs after libvirtd restart

src/lxc_driver.c

@@ -68,46 +68,48 @@ static void lxcDriverUnlock(lxc_driver_t *driver)
 }
 
 
-static int lxcProbe(void)
-{
-    if (getuid() != 0 ||
-        lxcContainerAvailable(0) < 0)
-        return 0;
-
-    return 1;
-}
-
 static virDrvOpenStatus lxcOpen(virConnectPtr conn,
                                 virConnectAuthPtr auth ATTRIBUTE_UNUSED,
                                 int flags ATTRIBUTE_UNUSED)
 {
-    if (lxc_driver == NULL)
-        goto declineConnection;
-
     /* Verify uri was specified */
     if (conn->uri == NULL) {
-        if (!lxcProbe())
+        if (lxc_driver == NULL)
-            goto declineConnection;
+            return VIR_DRV_OPEN_DECLINED;
 
         conn->uri = xmlParseURI("lxc:///");
         if (!conn->uri) {
             virReportOOMError(conn);
             return VIR_DRV_OPEN_ERROR;
         }
-    } else if (conn->uri->scheme == NULL ||
+    } else {
-               STRNEQ(conn->uri->scheme, "lxc")) {
+        if (conn->uri->scheme == NULL ||
-        goto declineConnection;
+            STRNEQ(conn->uri->scheme, "lxc"))
-    } else if (!lxcProbe()) {
+            return VIR_DRV_OPEN_DECLINED;
-        goto declineConnection;
-    }
 
+        /* Leave for remote driver */
+        if (conn->uri->server != NULL)
+            return VIR_DRV_OPEN_DECLINED;
+
+        /* If path isn't '/' then they typoed, tell them correct path */
+        if (STRNEQ(conn->uri->path, "/")) {
+            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("unexpected LXC URI path '%s', try lxc:///"),
+                     conn->uri->path);
+            return VIR_DRV_OPEN_ERROR;
+        }
+
+        /* URI was good, but driver isn't active */
+        if (lxc_driver == NULL) {
+            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("lxc state driver is not active"));
+            return VIR_DRV_OPEN_ERROR;
+        }
+    }
 
     conn->privateData = lxc_driver;
 
     return VIR_DRV_OPEN_SUCCESS;
-
-declineConnection:
-    return VIR_DRV_OPEN_DECLINED;
 }
 
 static int lxcClose(virConnectPtr conn)

src/openvz_driver.c

@@ -1070,36 +1070,59 @@ cleanup:
     return ret;
 }
 
-static int openvzProbe(void)
-{
-    if (geteuid() == 0 &&
-        virFileExists("/proc/vz"))
-        return 1;
-
-    return 0;
-}
-
 static virDrvOpenStatus openvzOpen(virConnectPtr conn,
                                    virConnectAuthPtr auth ATTRIBUTE_UNUSED,
                                    int flags ATTRIBUTE_UNUSED)
 {
     struct openvz_driver *driver;
-    if (!openvzProbe())
-        return VIR_DRV_OPEN_DECLINED;
 
     if (conn->uri == NULL) {
+        if (!virFileExists("/proc/vz"))
+            return VIR_DRV_OPEN_DECLINED;
+
+        if (access("/proc/vz", W_OK) < 0)
+            return VIR_DRV_OPEN_DECLINED;
+
         conn->uri = xmlParseURI("openvz:///system");
         if (conn->uri == NULL) {
-            openvzError(conn, VIR_ERR_NO_DOMAIN, NULL);
+            virReportOOMError(conn);
+            return VIR_DRV_OPEN_ERROR;
+        }
+    } else {
+        /* If scheme isn't 'openvz', then its for another driver */
+        if (conn->uri->scheme == NULL ||
+            STRNEQ (conn->uri->scheme, "openvz"))
+            return VIR_DRV_OPEN_DECLINED;
+
+        /* If server name is given, its for remote driver */
+        if (conn->uri->server != NULL)
+            return VIR_DRV_OPEN_DECLINED;
+
+        /* If path isn't /system, then they typoed, so tell them correct path */
+        if (conn->uri->path == NULL ||
+            STRNEQ (conn->uri->path, "/system")) {
+            openvzError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("unexpected OpenVZ URI path '%s', try openvz:///system"),
+                        conn->uri->path);
+            return VIR_DRV_OPEN_ERROR;
+        }
+
+        if (!virFileExists("/proc/vz")) {
+            openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
+                        _("OpenVZ control file /proc/vz does not exist"));
+            return VIR_DRV_OPEN_ERROR;
+        }
+
+        if (access("/proc/vz", W_OK) < 0) {
+            openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
+                        _("OpenVZ control file /proc/vz is not accessible"));
             return VIR_DRV_OPEN_ERROR;
         }
-    } else if (conn->uri->scheme == NULL ||
-               conn->uri->path == NULL ||
-               STRNEQ (conn->uri->scheme, "openvz") ||
-               STRNEQ (conn->uri->path, "/system")) {
-        return VIR_DRV_OPEN_DECLINED;
     }
 
+    /* We now know the URI is definitely for this driver, so beyond
+     * here, don't return DECLINED, always use ERROR */
+
     if (VIR_ALLOC(driver) < 0) {
         virReportOOMError(conn);
         return VIR_DRV_OPEN_ERROR;

src/qemu_driver.c

@@ -1743,62 +1743,61 @@ qemudMonitorCommand(const virDomainObjPtr vm,
 }
 
 
-/**
- * qemudProbe:
- *
- * Probe for the availability of the qemu driver, assume the
- * presence of QEmu emulation if the binaries are installed
- */
-static int qemudProbe(void)
-{
-    if ((virFileExists("/usr/bin/qemu")) ||
-        (virFileExists("/usr/bin/qemu-kvm")) ||
-        (virFileExists("/usr/bin/kvm")) ||
-        (virFileExists("/usr/bin/xenner")))
-        return 1;
-
-    return 0;
-}
 
 static virDrvOpenStatus qemudOpen(virConnectPtr conn,
                                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
                                   int flags ATTRIBUTE_UNUSED) {
     uid_t uid = getuid();
 
-    if (qemu_driver == NULL)
-        goto decline;
-
-    if (!qemudProbe())
-        goto decline;
-
     if (conn->uri == NULL) {
-        conn->uri = xmlParseURI(uid ? "qemu:///session" : "qemu:///system");
+        if (qemu_driver == NULL)
+            return VIR_DRV_OPEN_DECLINED;
+
+        conn->uri = xmlParseURI(uid == 0 ?
+                                "qemu:///system" :
+                                "qemu:///session");
         if (!conn->uri) {
             virReportOOMError(conn);
             return VIR_DRV_OPEN_ERROR;
         }
-    } else if (conn->uri->scheme == NULL ||
+    } else {
-               conn->uri->path == NULL)
+        /* If URI isn't 'qemu' its definitely not for us */
-        goto decline;
+        if (conn->uri->scheme == NULL ||
+            STRNEQ(conn->uri->scheme, "qemu"))
+            return VIR_DRV_OPEN_DECLINED;
 
-    if (STRNEQ (conn->uri->scheme, "qemu"))
+        /* Allow remote driver to deal with URIs with hostname server */
-        goto decline;
+        if (conn->uri->server != NULL)
+            return VIR_DRV_OPEN_DECLINED;
+
+        if (!uid) {
+            if (STRNEQ (conn->uri->path, "/system") &&
+                STRNEQ (conn->uri->path, "/session")) {
+                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                                 _("unexpected QEMU URI path '%s', try qemu:///system"),
+                                 conn->uri->path);
+                return VIR_DRV_OPEN_ERROR;
+            }
+        } else {
+            if (STRNEQ (conn->uri->path, "/session")) {
+                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                                 _("unexpected QEMU URI path '%s', try qemu:///session"),
+                                 conn->uri->path);
+                return VIR_DRV_OPEN_ERROR;
+            }
+        }
+
+        /* URI was good, but driver isn't active */
+        if (qemu_driver == NULL) {
+            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s",
+                             _("qemu state driver is not active"));
+            return VIR_DRV_OPEN_ERROR;
+        }
 
-    if (uid != 0) {
-        if (STRNEQ (conn->uri->path, "/session"))
-            goto decline;
-    } else { /* root */
-        if (STRNEQ (conn->uri->path, "/system") &&
-            STRNEQ (conn->uri->path, "/session"))
-            goto decline;
     }
-
     conn->privateData = qemu_driver;
 
     return VIR_DRV_OPEN_SUCCESS;
-
- decline:
-    return VIR_DRV_OPEN_DECLINED;
 }
 
 static int qemudClose(virConnectPtr conn) {

src/remote_internal.c

@@ -305,21 +305,28 @@ remoteForkDaemon(virConnectPtr conn)
 
 enum virDrvOpenRemoteFlags {
     VIR_DRV_OPEN_REMOTE_RO = (1 << 0),
-    VIR_DRV_OPEN_REMOTE_UNIX = (1 << 1),
+    VIR_DRV_OPEN_REMOTE_USER      = (1 << 1), /* Use the per-user socket path */
-    VIR_DRV_OPEN_REMOTE_USER = (1 << 2),
+    VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 2), /* Autostart a per-user daemon */
-    VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 3),
 };
 
-/* What transport? */
-enum {
-    trans_tls,
-    trans_unix,
-    trans_ssh,
-    trans_ext,
-    trans_tcp,
-} transport;
-
 
+/*
+ * URIs that this driver needs to handle:
+ *
+ * The easy answer:
+ *   - Everything that no one else has yet claimed, but nothing if
+ *     we're inside the libvirtd daemon
+ *
+ * The hard answer:
+ *   - Plain paths (///var/lib/xen/xend-socket)  -> UNIX domain socket
+ *   - xxx://servername/      -> TLS connection
+ *   - xxx+tls://servername/  -> TLS connection
+ *   - xxx+tls:///            -> TLS connection to localhost
+ *   - xxx+tcp://servername/  -> TCP connection
+ *   - xxx+tcp:///            -> TCP connection to localhost
+ *   - xxx+unix:///           -> UNIX domain socket
+ *   - xxx:///                -> UNIX domain socket
+ */
 static int
 doRemoteOpen (virConnectPtr conn,
               struct private_data *priv,
@@ -328,37 +335,51 @@ doRemoteOpen (virConnectPtr conn,
 {
     int wakeupFD[2] = { -1, -1 };
     char *transport_str = NULL;
+    enum {
+        trans_tls,
+        trans_unix,
+        trans_ssh,
+        trans_ext,
+        trans_tcp,
+    } transport;
+
+    /* We handle *ALL*  URIs here. The caller has rejected any
+     * URIs we don't care about */
 
     if (conn->uri) {
-        if (!conn->uri->scheme)
+        if (!conn->uri->scheme) {
-            return VIR_DRV_OPEN_DECLINED;
+            /* This is the ///var/lib/xen/xend-socket local path style */
-
-        transport_str = get_transport_from_scheme (conn->uri->scheme);
-
-        if (!transport_str || STRCASEEQ (transport_str, "tls"))
-            transport = trans_tls;
-        else if (STRCASEEQ (transport_str, "unix"))
             transport = trans_unix;
-        else if (STRCASEEQ (transport_str, "ssh"))
+        } else {
-            transport = trans_ssh;
+            transport_str = get_transport_from_scheme (conn->uri->scheme);
-        else if (STRCASEEQ (transport_str, "ext"))
+
-            transport = trans_ext;
+            if (!transport_str) {
-        else if (STRCASEEQ (transport_str, "tcp"))
+                if (conn->uri->server)
-            transport = trans_tcp;
+                    transport = trans_tls;
-        else {
+                else
-            error (conn, VIR_ERR_INVALID_ARG,
+                    transport = trans_unix;
-                   _("remote_open: transport in URL not recognised "
+            } else {
-                     "(should be tls|unix|ssh|ext|tcp)"));
+                if (STRCASEEQ (transport_str, "tls"))
-            return VIR_DRV_OPEN_ERROR;
+                    transport = trans_tls;
+                else if (STRCASEEQ (transport_str, "unix"))
+                    transport = trans_unix;
+                else if (STRCASEEQ (transport_str, "ssh"))
+                    transport = trans_ssh;
+                else if (STRCASEEQ (transport_str, "ext"))
+                    transport = trans_ext;
+                else if (STRCASEEQ (transport_str, "tcp"))
+                    transport = trans_tcp;
+                else {
+                    error (conn, VIR_ERR_INVALID_ARG,
+                           _("remote_open: transport in URL not recognised "
+                             "(should be tls|unix|ssh|ext|tcp)"));
+                    return VIR_DRV_OPEN_ERROR;
+                }
+            }
         }
-    }
+    } else {
-
+        /* No URI, then must be probing so use UNIX socket */
-    if (!transport_str) {
+        transport = trans_unix;
-        if ((!conn->uri || !conn->uri->server) &&
-            (flags & VIR_DRV_OPEN_REMOTE_UNIX))
-            transport = trans_unix;
-        else
-            return VIR_DRV_OPEN_DECLINED; /* Decline - not a remote URL. */
     }
 
     /* Local variables which we will initialise. These can
@@ -455,8 +476,9 @@ doRemoteOpen (virConnectPtr conn,
 
         /* Construct the original name. */
         if (!name) {
-            if (STREQ(conn->uri->scheme, "remote") ||
+            if (conn->uri->scheme &&
-                STRPREFIX(conn->uri->scheme, "remote+")) {
+                (STREQ(conn->uri->scheme, "remote") ||
+                 STRPREFIX(conn->uri->scheme, "remote+"))) {
                 /* Allow remote serve to probe */
                 name = strdup("");
             } else {
@@ -580,7 +602,7 @@ doRemoteOpen (virConnectPtr conn,
 
         freeaddrinfo (res);
         virReportSystemError(conn, saved_errno,
-                             _("unable to connect to '%s'"),
+                             _("unable to connect to libvirtd at '%s'"),
                              priv->hostname);
         goto failed;
 
@@ -925,7 +947,6 @@ remoteOpenSecondaryDriver(virConnectPtr conn,
 
     if (flags & VIR_CONNECT_RO)
         rflags |= VIR_DRV_OPEN_REMOTE_RO;
-    rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
 
     ret = doRemoteOpen(conn, *priv, auth, rflags);
     if (ret != VIR_DRV_OPEN_SUCCESS) {
@@ -956,19 +977,6 @@ remoteOpen (virConnectPtr conn,
     if (flags & VIR_CONNECT_RO)
         rflags |= VIR_DRV_OPEN_REMOTE_RO;
 
-    /*
-     * If no servername is given, and no +XXX
-     * transport is listed, then force to a
-     * local UNIX socket connection
-     */
-    if (conn->uri &&
-        !conn->uri->server &&
-        conn->uri->scheme &&
-        !strchr(conn->uri->scheme, '+')) {
-        DEBUG0("Auto-remote UNIX socket");
-        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
-    }
-
     /*
      * If no servername is given, and no +XXX
      * transport is listed, or transport is unix,
@@ -996,7 +1004,6 @@ remoteOpen (virConnectPtr conn,
      */
     if (!conn->uri) {
         DEBUG0("Auto-probe remote URI");
-        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
 #ifndef __sun
         if (getuid() > 0) {
             DEBUG0("Auto-spawn user daemon instance");

src/test.c

@@ -639,9 +639,6 @@ static virDrvOpenStatus testOpen(virConnectPtr conn,
     if (conn->uri->server)
         return VIR_DRV_OPEN_DECLINED;
 
-    if (conn->uri->server)
-        return VIR_DRV_OPEN_DECLINED;
-
     /* From this point on, the connection is for us. */
     if (!conn->uri->path
         || conn->uri->path[0] == '\0'

src/uml_driver.c

@@ -913,38 +913,56 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn,
                                 int flags ATTRIBUTE_UNUSED) {
     uid_t uid = getuid();
 
-    if (uml_driver == NULL)
+    if (conn->uri == NULL) {
-        goto decline;
+        if (uml_driver == NULL)
+            return VIR_DRV_OPEN_DECLINED;
 
-    if (conn->uri != NULL) {
+        conn->uri = xmlParseURI(uid == 0 ?
-        if (conn->uri->scheme == NULL || conn->uri->path == NULL)
+                                "uml:///system" :
-            goto decline;
+                                "uml:///session");
-
-        if (STRNEQ (conn->uri->scheme, "uml"))
-            goto decline;
-
-        if (uid != 0) {
-            if (STRNEQ (conn->uri->path, "/session"))
-                goto decline;
-        } else { /* root */
-            if (STRNEQ (conn->uri->path, "/system") &&
-                STRNEQ (conn->uri->path, "/session"))
-                goto decline;
-        }
-    } else {
-        conn->uri = xmlParseURI(uid ? "uml:///session" : "uml:///system");
         if (!conn->uri) {
             virReportOOMError(conn);
             return VIR_DRV_OPEN_ERROR;
         }
+    } else {
+        if (conn->uri->scheme == NULL ||
+            STRNEQ (conn->uri->scheme, "uml"))
+            return VIR_DRV_OPEN_DECLINED;
+
+        /* Allow remote driver to deal with URIs with hostname server */
+        if (conn->uri->server != NULL)
+            return VIR_DRV_OPEN_DECLINED;
+
+
+        /* Check path and tell them correct path if they made a mistake */
+        if (uid == 0) {
+            if (STRNEQ (conn->uri->path, "/system") &&
+                STRNEQ (conn->uri->path, "/session")) {
+                umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                               _("unexpected UML URI path '%s', try uml:///system"),
+                               conn->uri->path);
+                return VIR_DRV_OPEN_ERROR;
+            }
+        } else {
+            if (STRNEQ (conn->uri->path, "/session")) {
+                umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                               _("unexpected UML URI path '%s', try uml:///session"),
+                               conn->uri->path);
+                return VIR_DRV_OPEN_ERROR;
+            }
+        }
+
+        /* URI was good, but driver isn't active */
+        if (uml_driver == NULL) {
+            umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("uml state driver is not active"));
+            return VIR_DRV_OPEN_ERROR;
+        }
     }
 
     conn->privateData = uml_driver;
 
     return VIR_DRV_OPEN_SUCCESS;
-
- decline:
-    return VIR_DRV_OPEN_DECLINED;
 }
 
 static int umlClose(virConnectPtr conn) {

src/virterror.c

@@ -737,9 +737,9 @@ virErrorMsg(virErrorNumber error, const char *info)
             break;
         case VIR_ERR_NO_CONNECT:
             if (info == NULL)
-                errmsg = _("could not connect to hypervisor");
+                errmsg = _("no hypervisor driver available");
             else
-                errmsg = _("could not connect to %s");
+                errmsg = _("no hypervisor driver available for %s");
             break;
         case VIR_ERR_INVALID_CONN:
             if (info == NULL)

src/xen_unified.c

@@ -241,25 +241,46 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, int flags)
             virReportOOMError (NULL);
             return VIR_DRV_OPEN_ERROR;
         }
+    } else {
+        if (conn->uri->scheme) {
+            /* Decline any scheme which isn't "xen://" or "http://". */
+            if (STRCASENEQ(conn->uri->scheme, "xen") &&
+                STRCASENEQ(conn->uri->scheme, "http"))
+                return VIR_DRV_OPEN_DECLINED;
+
+
+            /* Return an error if the path isn't '' or '/' */
+            if (conn->uri->path &&
+                STRNEQ(conn->uri->path, "") &&
+                STRNEQ(conn->uri->path, "/")) {
+                xenUnifiedError(NULL, VIR_ERR_INTERNAL_ERROR,
+                                _("unexpected Xen URI path '%s', try xen:///"),
+                                conn->uri->path);
+                return VIR_DRV_OPEN_ERROR;
+            }
+
+            /* Decline any xen:// URI with a server specified, allowing remote
+             * driver to handle, but keep any http:/// URIs */
+            if (STRCASEEQ(conn->uri->scheme, "xen") &&
+                conn->uri->server)
+                return VIR_DRV_OPEN_DECLINED;
+        } else {
+            /* Special case URI for Xen driver only:
+             *
+             * Treat a plain path as a Xen UNIX socket path, and give
+             * error unless path is absolute
+             */
+            if (!conn->uri->path || conn->uri->path[0] != '/') {
+                xenUnifiedError(NULL, VIR_ERR_INTERNAL_ERROR,
+                                _("unexpected Xen URI path '%s', try ///var/lib/xen/xend-socket"),
+                                NULLSTR(conn->uri->path));
+                return VIR_DRV_OPEN_ERROR;
+            }
+        }
     }
 
-    /* Refuse any scheme which isn't "xen://" or "http://". */
+    /* We now know the URI is definitely for this driver, so beyond
-    if (conn->uri->scheme &&
+     * here, don't return DECLINED, always use ERROR */
-        STRCASENEQ(conn->uri->scheme, "xen") &&
-        STRCASENEQ(conn->uri->scheme, "http"))
-        return VIR_DRV_OPEN_DECLINED;
-
-    /* xmlParseURI will parse a naked string like "foo" as a URI with
-     * a NULL scheme.  That's not useful for us because we want to only
-     * allow full pathnames (eg. ///var/lib/xen/xend-socket).  Decline
-     * anything else.
-     */
-    if (!conn->uri->scheme && (!conn->uri->path || conn->uri->path[0] != '/'))
-        return VIR_DRV_OPEN_DECLINED;
-
-    /* Refuse any xen:// URI with a server specified - allow remote to do it */
-    if (conn->uri->scheme && STRCASEEQ(conn->uri->scheme, "xen") && conn->uri->server)
-        return VIR_DRV_OPEN_DECLINED;
 
     /* Allocate per-connection private data. */
     if (VIR_ALLOC(priv) < 0) {

src/xend_internal.c

@@ -2927,10 +2927,13 @@ xenDaemonOpen(virConnectPtr conn,
             xend_detect_config_version(conn) == -1)
             goto failed;
     } else if (STRCASEEQ (conn->uri->scheme, "http")) {
-        if (virAsprintf(&port, "%d", conn->uri->port) == -1)
+        if (conn->uri->port &&
+            virAsprintf(&port, "%d", conn->uri->port) == -1)
             goto failed;
 
-        if (xenDaemonOpen_tcp(conn, conn->uri->server, port) < 0 ||
+        if (xenDaemonOpen_tcp(conn,
+                              conn->uri->server ? conn->uri->server : "localhost",
+                              port ? port : "8000") < 0 ||
             xend_detect_config_version(conn) == -1)
             goto failed;
     } else {