5:22 a.m.
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 :|