[libvirt] PATCH: Improve URI error reporting

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/