From d2c9fe850b5c158ec7c1f1f9c3c6ab6424239ad5 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 12 Jun 2009 12:06:15 +0000 Subject: [PATCH] Improve error reporting for virConnectOpen URIs --- ChangeLog | 14 +++++ src/lxc_driver.c | 48 +++++++++-------- src/openvz_driver.c | 57 ++++++++++++++------ src/qemu_driver.c | 77 +++++++++++++-------------- src/remote_internal.c | 121 ++++++++++++++++++++++-------------------- src/test.c | 3 -- src/uml_driver.c | 62 ++++++++++++++-------- src/virterror.c | 4 +- src/xen_unified.c | 55 +++++++++++++------ src/xend_internal.c | 7 ++- 10 files changed, 266 insertions(+), 182 deletions(-) diff --git a/ChangeLog b/ChangeLog index 53d8faa4ee..4ef02d263f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +Thu Jun 12 13:06:42 BST 2009 Daniel P. Berrange + + 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 Fix re-detection of transient VMs after libvirtd restart diff --git a/src/lxc_driver.c b/src/lxc_driver.c index 6cdcbf3a8a..b286425ab8 100644 --- a/src/lxc_driver.c +++ b/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()) - goto declineConnection; + if (lxc_driver == NULL) + 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 || - STRNEQ(conn->uri->scheme, "lxc")) { - goto declineConnection; - } else if (!lxcProbe()) { - goto declineConnection; - } + } else { + if (conn->uri->scheme == NULL || + STRNEQ(conn->uri->scheme, "lxc")) + return VIR_DRV_OPEN_DECLINED; + /* 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) diff --git a/src/openvz_driver.c b/src/openvz_driver.c index 4c0fc5f87c..85f6173005 100644 --- a/src/openvz_driver.c +++ b/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; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 5f94bed7ee..4a02840eab 100644 --- a/src/qemu_driver.c +++ b/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 || - conn->uri->path == NULL) - goto decline; + } else { + /* If URI isn't 'qemu' its definitely not for us */ + if (conn->uri->scheme == NULL || + STRNEQ(conn->uri->scheme, "qemu")) + return VIR_DRV_OPEN_DECLINED; - if (STRNEQ (conn->uri->scheme, "qemu")) - goto decline; + /* Allow remote driver to deal with URIs with hostname server */ + 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) { diff --git a/src/remote_internal.c b/src/remote_internal.c index 0363ba4145..e4fa1569c7 100644 --- a/src/remote_internal.c +++ b/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 << 2), - VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 3), + VIR_DRV_OPEN_REMOTE_USER = (1 << 1), /* Use the per-user socket path */ + VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 2), /* Autostart a per-user daemon */ }; -/* 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) - return VIR_DRV_OPEN_DECLINED; - - 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")) + if (!conn->uri->scheme) { + /* This is the ///var/lib/xen/xend-socket local path style */ 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 { + transport_str = get_transport_from_scheme (conn->uri->scheme); + + if (!transport_str) { + if (conn->uri->server) + transport = trans_tls; + else + transport = trans_unix; + } else { + if (STRCASEEQ (transport_str, "tls")) + 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; + } + } } - } - - if (!transport_str) { - 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. */ + } else { + /* No URI, then must be probing so use UNIX socket */ + transport = trans_unix; } /* 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") || - STRPREFIX(conn->uri->scheme, "remote+")) { + if (conn->uri->scheme && + (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"); diff --git a/src/test.c b/src/test.c index ba9ad6620c..7dc0840e61 100644 --- a/src/test.c +++ b/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' diff --git a/src/uml_driver.c b/src/uml_driver.c index 2df342c83a..d3e829ad91 100644 --- a/src/uml_driver.c +++ b/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) - goto decline; + if (conn->uri == NULL) { + if (uml_driver == NULL) + return VIR_DRV_OPEN_DECLINED; - if (conn->uri != NULL) { - if (conn->uri->scheme == NULL || conn->uri->path == NULL) - goto decline; - - 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"); + conn->uri = xmlParseURI(uid == 0 ? + "uml:///system" : + "uml:///session"); 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) { diff --git a/src/virterror.c b/src/virterror.c index b1bf391133..b1c9ba4be0 100644 --- a/src/virterror.c +++ b/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) diff --git a/src/xen_unified.c b/src/xen_unified.c index 57efb40a14..2535f99e8e 100644 --- a/src/xen_unified.c +++ b/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://". */ - if (conn->uri->scheme && - 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; + /* We now know the URI is definitely for this driver, so beyond + * here, don't return DECLINED, always use ERROR */ /* Allocate per-connection private data. */ if (VIR_ALLOC(priv) < 0) { diff --git a/src/xend_internal.c b/src/xend_internal.c index 94aaa42e87..b211bb6293 100644 --- a/src/xend_internal.c +++ b/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 {