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