On Wed, Jun 03, 2009 at 05:31:13PM +0100, Daniel P. Berrange wrote:
> There are currently far too many cases where a correct URI returns an
> generic 'failed to connect to hypervisor' error message. This gives the
> user no idea why they could not connect. Of particular importance is
> that they cannot distinguish a correct URI + plus a driver problem, vs
> incorrect URI. Consider the following examples
[...]
> The core problem here is that far too many places are using the return
> code VIR_DRV_OPEN_DECLINED instead of VIR_DRV_OPEN_ERROR. The former
> provides no way to give any error info to the user. With this patch
> I have taken the view that a driver must *only* ever use the return
> code VIR_DRV_OPEN_DECLINED when:
>
> - Auto-probe of a driver URI, and this driver is not active
> - Explicit URI with 'server' specified
> - URI scheme does not match the driver
okay
> The remote driver is special in that it *must* accept all URIs given to
As long as we check first it's a correct URI...
> it, no matter what, unless of course it is running inside the daemon
> already. The result is that if you give a correct URI, but there is a
> real problem opening the driver you are now guarenteed to get a useful
> error message back. Consider the same examples again
yup that looks way better
[...]
> -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;
> +
Okay I was confused at first about dropping geteuid() == 0 check but
it's a more specific check,
> conn->uri = xmlParseURI("openvz:///system");
> if (conn->uri == NULL) {
> - openvzError(conn, VIR_ERR_NO_DOMAIN, NULL);
> + virReportOOMError(conn);
Hum ... okay .
[...]
> --- a/src/remote_internal.c Wed Jun 03 15:37:52 2009 +0100
> +++ b/src/remote_internal.c Wed Jun 03 17:31:17 2009 +0100
> @@ -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),
> -};
> -
> -/* What transport? */
> -enum {
> - trans_tls,
> - trans_unix,
> - trans_ssh,
> - trans_ext,
> - trans_tcp,
> -} transport;
> -
> -
> + 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 */
> +};
> +
> +
> +/*
> + * 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;
hum ... I would have expected this to remain global somehow, but
thinking about it, fine :-)
Okay, the changes are larger than I would have expected but it's
similar for all drivers. Looks fine, ACK
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list