Re: [libvirt] PATCH: Another attempt to fix vbox driver open
On Mon, May 11, 2009 at 05:35:36PM +0200, Pritesh Kothari wrote: - If the user gives a URI with a vbox:/// prefix, we should always handle it, unless a 'server' is set when we leave it to the remote driver - If an invalid path is given we must give back a real error code - If after deciding the URI is for us, any initialization fails we must raise an error. - If the vbox glue layer is missing, we should still raise errors for requested URIs, so user knows their URI is correct. To do this, I've taken the approach of registering a dummy vbox driver if the glue layer is missing. This just parses the URI and always returns an error for any vbox:// URIs that would otherwise work ACK. works fine for 2.2.* as well as internal 2.5* development versions. Thanks, I've committed this now 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Another attempt to fix vbox driver open
On Fri, May 08, 2009 at 05:42:07PM +0100, Daniel P. Berrange wrote: The patches we just applied for the VirtualBox open method still were not quite right. It would return VIR_DRV_OPEN_DECLINED when uri==NULL, but before doing so it would have set conn-uri to vbox:///session. So even though it declined the connection, all the later drivers would now ignore it. Also, it now returns DECLINED for some real errors that should be reported to the user. Here's an alternative idea I've had for trying to address this. Some goals: - If the user gives a URI with a vbox:/// prefix, we should always handle it, unless a 'server' is set when we leave it to the remote driver - If an invalid path is given we must give back a real error code - If after deciding the URI is for us, any initialization fails we must raise an error. - If the vbox glue layer is missing, we should still raise errors for requested URIs, so user knows their URI is correct. Looks very sensible to me and much nicer than my original suggestion to move VBoxCGlueInit() early into vboxOpen. [..snip..] + _(no VirtualBox drviver path specified (try vbox:///session))); s/drviver/driver/ Cheers, -- Guido -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Another attempt to fix vbox driver open
On Fri, May 08, 2009 at 05:42:07PM +0100, Daniel P. Berrange wrote:
The patches we just applied for the VirtualBox open method still were
not quite right. It would return VIR_DRV_OPEN_DECLINED when uri==NULL,
but before doing so it would have set conn-uri to vbox:///session. So
even though it declined the connection, all the later drivers would now
ignore it. Also, it now returns DECLINED for some real errors that
should be reported to the user.
Here's an alternative idea I've had for trying to address this. Some
goals:
- If the user gives a URI with a vbox:/// prefix, we should always
handle it, unless a 'server' is set when we leave it to the remote
driver
- If an invalid path is given we must give back a real error code
- If after deciding the URI is for us, any initialization fails
we must raise an error.
- If the vbox glue layer is missing, we should still raise errors
for requested URIs, so user knows their URI is correct.
All sounds good !
To do this, I've taken the approach of registering a dummy vbox driver
if the glue layer is missing. This just parses the URI and always returns
an error for any vbox:// URIs that would otherwise work
Interesting ...
[...]
-/* vboxRegister() shouldn't fail as that will render libvirt unless.
- * So, we use the v2.2 driver as a fallback/dummy.
+/*
+ * If the glue layer won' initialize, we register a driver
won't or does not :-)
+ * with a dummy open method, so we can report nicer errors
+ * if the user requests a vbox:// URI which we know won't
+ * ever work
will never ?
[...]
+if (conn-uri == NULL ||
+conn-uri-scheme == NULL ||
+STRNEQ (conn-uri-scheme, vbox) ||
+conn-uri-server != NULL)
+return VIR_DRV_OPEN_DECLINED;
Hum, we accept NULL to indicate Xen or KVM/QEmu by default.
Maybe if none of them is available, we should allow NULL to start
the VirtualBox driver. It could be useful say on MacOS, or
just if VirtualBox is installed on Linux while we know KVM is not.
+if (conn-uri-path == NULL || STREQ(conn-uri-path, )) {
+vboxError(conn, VIR_ERR_INTERNAL_ERROR, %s,
+ _(no VirtualBox drviver path specified (try
vbox:///session)));
+return VIR_DRV_OPEN_ERROR;
+}
[...]
+static virDriver vboxDriverDummy = {
+VIR_DRV_VBOX,
+VBOX,
+.open = vboxOpenDummy,
+};
In that case the new style initilization makes sense.
@@ -291,13 +281,13 @@ static int vboxExtractVersion(virConnect
}
static void vboxUninitialize(vboxGlobalData *data) {
+if (!data)
+return;
+
if (data-pFuncs)
data-pFuncs-pfnComUninitialize();
VBoxCGlueTerm();
-if (!data)
-return;
-
virDomainObjListFree(data-domains);
virCapabilitiesFree(data-caps);
VIR_FREE(data);
That's a bug fix which should be applied in any case.
@@ -306,52 +296,62 @@ static void vboxUninitialize(vboxGlobalD
static virDrvOpenStatus vboxOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags ATTRIBUTE_UNUSED) {
-vboxGlobalData *data;
+vboxGlobalData *data = NULL;
uid_t uid = getuid();
if (conn-uri == NULL) {
conn-uri = xmlParseURI(uid ? vbox:///session : vbox:///system);
Ah, okay, so NULL is still accepted, this is getting complex
If Pritesh can review and test the patch, I'm fine with it.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Another attempt to fix vbox driver open
On Mon, May 11, 2009 at 04:27:36PM +0200, Daniel Veillard wrote: On Fri, May 08, 2009 at 05:42:07PM +0100, Daniel P. Berrange wrote: [...] +if (conn-uri == NULL || +conn-uri-scheme == NULL || +STRNEQ (conn-uri-scheme, vbox) || +conn-uri-server != NULL) +return VIR_DRV_OPEN_DECLINED; Hum, we accept NULL to indicate Xen or KVM/QEmu by default. Maybe if none of them is available, we should allow NULL to start the VirtualBox driver. It could be useful say on MacOS, or just if VirtualBox is installed on Linux while we know KVM is not. This block of code is in the vboxOpenDummy() function. In this scenario we have already determined that the VirtualBox RPC libraries are not available. Hence there is no need to perform a probe when uri == NULL here. The other real vboxOpen() method you noticed later in the patch do still handle NULL 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Another attempt to fix vbox driver open
- If the user gives a URI with a vbox:/// prefix, we should always handle it, unless a 'server' is set when we leave it to the remote driver - If an invalid path is given we must give back a real error code - If after deciding the URI is for us, any initialization fails we must raise an error. - If the vbox glue layer is missing, we should still raise errors for requested URIs, so user knows their URI is correct. To do this, I've taken the approach of registering a dummy vbox driver if the glue layer is missing. This just parses the URI and always returns an error for any vbox:// URIs that would otherwise work ACK. works fine for 2.2.* as well as internal 2.5* development versions. Regards, Pritesh -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] PATCH: Another attempt to fix vbox driver open
The patches we just applied for the VirtualBox open method still were
not quite right. It would return VIR_DRV_OPEN_DECLINED when uri==NULL,
but before doing so it would have set conn-uri to vbox:///session. So
even though it declined the connection, all the later drivers would now
ignore it. Also, it now returns DECLINED for some real errors that
should be reported to the user.
Here's an alternative idea I've had for trying to address this. Some
goals:
- If the user gives a URI with a vbox:/// prefix, we should always
handle it, unless a 'server' is set when we leave it to the remote
driver
- If an invalid path is given we must give back a real error code
- If after deciding the URI is for us, any initialization fails
we must raise an error.
- If the vbox glue layer is missing, we should still raise errors
for requested URIs, so user knows their URI is correct.
To do this, I've taken the approach of registering a dummy vbox driver
if the glue layer is missing. This just parses the URI and always returns
an error for any vbox:// URIs that would otherwise work
Daniel
Index: src/vbox/vbox_driver.c
===
RCS file: /data/cvs/libvirt/src/vbox/vbox_driver.c,v
retrieving revision 1.2
diff -u -p -r1.2 vbox_driver.c
--- src/vbox/vbox_driver.c 6 May 2009 13:51:19 - 1.2
+++ src/vbox/vbox_driver.c 8 May 2009 16:35:57 -
@@ -34,6 +34,7 @@
#include logging.h
#include vbox_driver.h
#include vbox_XPCOMCGlue.h
+#include virterror_internal.h
#define VIR_FROM_THIS VIR_FROM_VBOX
@@ -43,15 +44,25 @@ extern virDriver vbox22Driver;
extern virDriver vbox25Driver;
#endif
+static virDriver vboxDriverDummy;
+
+#define VIR_FROM_THIS VIR_FROM_VBOX
+
+#define vboxError(conn, code, fmt...) \
+virReportErrorHelper(conn, VIR_FROM_VBOX, code, __FILE__, \
+__FUNCTION__, __LINE__, fmt)
int vboxRegister(void) {
virDriverPtrdriver;
uint32_tuVersion;
-/* vboxRegister() shouldn't fail as that will render libvirt unless.
- * So, we use the v2.2 driver as a fallback/dummy.
+/*
+ * If the glue layer won' initialize, we register a driver
+ * with a dummy open method, so we can report nicer errors
+ * if the user requests a vbox:// URI which we know won't
+ * ever work
*/
-driver= vbox22Driver;
+driver= vboxDriverDummy;
/* Init the glue and get the API version. */
if (VBoxCGlueInit() == 0) {
@@ -79,7 +90,7 @@ int vboxRegister(void) {
}
} else {
-DEBUG0(VBoxCGlueInit failed);
+DEBUG0(VBoxCGlueInit failed, using dummy driver);
}
if (virRegisterDriver(driver) 0)
@@ -87,3 +98,46 @@ int vboxRegister(void) {
return 0;
}
+
+static virDrvOpenStatus vboxOpenDummy(virConnectPtr conn,
+ virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ int flags ATTRIBUTE_UNUSED) {
+uid_t uid = getuid();
+
+if (conn-uri == NULL ||
+conn-uri-scheme == NULL ||
+STRNEQ (conn-uri-scheme, vbox) ||
+conn-uri-server != NULL)
+return VIR_DRV_OPEN_DECLINED;
+
+if (conn-uri-path == NULL || STREQ(conn-uri-path, )) {
+vboxError(conn, VIR_ERR_INTERNAL_ERROR, %s,
+ _(no VirtualBox drviver path specified (try
vbox:///session)));
+return VIR_DRV_OPEN_ERROR;
+}
+
+if (uid != 0) {
+if (STRNEQ (conn-uri-path, /session)) {
+vboxError(conn, VIR_ERR_INTERNAL_ERROR,
+ _(unknown driver path '%s' specified (try
vbox:///session)), conn-uri-path);
+return VIR_DRV_OPEN_ERROR;
+}
+} else { /* root */
+if (STRNEQ (conn-uri-path, /system)
+STRNEQ (conn-uri-path, /session)) {
+vboxError(conn, VIR_ERR_INTERNAL_ERROR,
+ _(unknown driver path '%s' specified (try
vbox:///system)), conn-uri-path);
+return VIR_DRV_OPEN_ERROR;
+}
+}
+
+vboxError(conn, VIR_ERR_INTERNAL_ERROR, %s,
+ _(unable to initialize VirtualBox driver API));
+return VIR_DRV_OPEN_ERROR;
+}
+
+static virDriver vboxDriverDummy = {
+VIR_DRV_VBOX,
+VBOX,
+.open = vboxOpenDummy,
+};
Index: src/vbox/vbox_tmpl.c
===
RCS file: /data/cvs/libvirt/src/vbox/vbox_tmpl.c,v
retrieving revision 1.5
diff -u -p -r1.5 vbox_tmpl.c
--- src/vbox/vbox_tmpl.c8 May 2009 10:18:26 - 1.5
+++ src/vbox/vbox_tmpl.c8 May 2009 16:35:57 -
@@ -216,16 +216,6 @@ no_memory:
}
static int vboxInitialize(virConnectPtr conn, vboxGlobalData *data) {
-
-if (VBoxCGlueInit() != 0) {
-vboxError(conn, VIR_ERR_INTERNAL_ERROR, Can't Initialize VirtualBox,
VBoxCGlueInit failed.);
-goto cleanup;
-}
-
-/* This is for when glue init failed
