[libvirt] PATCH: Improve URI error reporting

[Daniel P. Berrange]

On Thu, Jun 11, 2009 at 05:49:55PM +0200, Daniel Veillard wrote:
> 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...

Well the URI is parsed by libxml already so its good enough for the
remote driver. It'll just pass it onto the libvirtd daemon, where a
real driver will accept or decline it. So the remote driver doesn't
need todo any validation itself really.

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

And geteuid() will soon be wrong when the libvirtd daemon runs as
non-root, but with appropriate capabilities to access /proc/vz.


> >  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 :-)

Its not entirely clear at first glance, but the original code was
declaring a global variable 'transport' with an anonymous enum.
And this global variable was used from the 'doRemoteOpen' method.
This is totally & utterly unsafe, it should always have been a
local variable :-)

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|