[libvirt] QEMU/KVM auto-detection bug
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is: if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; } So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment). In point of fact, this whole auto-detection thing is fragile. We got away with it before because the qemuOpen() routine bombs out before doing the whole conn->uri == NULL check, but in general, it's very easy for one buggy driver to mess up auto-detection for all drivers. Any thoughts about where to go here? We should obviously fix the vboxOpen() routine to not mess with the URI if it's going to fail. Additionally, I was thinking that maybe we should reset conn->uri to NULL in each iteration of the auto-detection loop. While that might leak memory for buggy drivers, it should at least make this whole thing less fragile. Thoughts? -- Chris Lalancette
On Tuesday 12 May 2009 11:36:50 Chris Lalancette wrote:
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is:
if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; }
So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment).
this seems to be common case in all other drivers also, for example uml, openvz, etc does the same and not to mention that qemu driver does it also, so i guess the fix should apply to all and not just vbox driver. Regards, Pritesh
Pritesh Kothari wrote:
On Tuesday 12 May 2009 11:36:50 Chris Lalancette wrote:
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is:
if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; }
So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment).
this seems to be common case in all other drivers also, for example uml, openvz, etc does the same and not to mention that qemu driver does it also, so i guess the fix should apply to all and not just vbox driver.
Well, yes and no. They all do it, but only *after* doing their probe routines. At least, I've never had a problem with auto-detection until I enabled vbox to build in my libvirt config. Like I said, though, the whole thing is fragile, and yes, I would prefer something that would help all of the drivers. Resetting the conn->uri pointer to NULL in each iteration of the loop is tempting and easy, but on the other hand, it could easily cause resource leaks in drivers to go undetected. -- Chris Lalancette
On Tuesday 12 May 2009 11:55:17 Chris Lalancette wrote:
Pritesh Kothari wrote:
On Tuesday 12 May 2009 11:36:50 Chris Lalancette wrote:
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is:
if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; }
So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment).
this seems to be common case in all other drivers also, for example uml, openvz, etc does the same and not to mention that qemu driver does it also, so i guess the fix should apply to all and not just vbox driver.
Well, yes and no. They all do it, but only *after* doing their probe routines. At least, I've never had a problem with auto-detection until I enabled vbox to build in my libvirt config.
You are right, the probe routines are before in other drivers, but in case of vbox it does initializations and not just probing and thus it can initialize only after all of uri conditions are satisfied. now this is a case where the driver needs to do more then just probing and vbox seems to be the very first driver to do it, as you have mentioned it can be solved by resetting uri, i think a more generic solution is needed here where if new drivers are added later on and need to do initializations and not just probing then they should not use the hack but the generic solution. So any thought on it? Regards, Pritesh
On Tue, May 12, 2009 at 11:36:50AM +0200, Chris Lalancette wrote:
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is:
if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; }
So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment).
Please re-test with the patch I posted yesterday http://www.redhat.com/archives/libvir-list/2009-May/msg00198.html
In point of fact, this whole auto-detection thing is fragile. We got away with it before because the qemuOpen() routine bombs out before doing the whole conn->uri == NULL check, but in general, it's very easy for one buggy driver to mess up auto-detection for all drivers.
The general rule is that a driver should not set conn->uri to non-NULL until *after* it has determined that it can handle it. The problem here was that virtualbox driver was setting the uri to non-NULL, and only then seeing if virtalbox was available. My patch fixes that logic ordering bug 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 :|
Daniel Berrange wrote:
On Tue, May 12, 2009 at 11:36:50AM +0200, Chris Lalancette wrote:
All, Even with Guido/Pritesh's recent changes to the vbox open routine, the auto-detection in libvirt is currently broken. What happens is that in src/libvirt.c:do_open(), at the start of the loop to auto-detect drivers, ret->uri is NULL. As each driver declines, it remains NULL. However, the very first thing the vboxOpen() routine does is:
if (conn->uri == NULL) { conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { return VIR_DRV_OPEN_ERROR; }
So, any driver that is trying to auto-probe after vboxOpen sees a conn->uri with a scheme of vbox and a path of system/session, and they all fail (including Qemu, which is what I'm mostly concerned with at the moment).
Please re-test with the patch I posted yesterday
http://www.redhat.com/archives/libvir-list/2009-May/msg00198.html
In point of fact, this whole auto-detection thing is fragile. We got away with it before because the qemuOpen() routine bombs out before doing the whole conn->uri == NULL check, but in general, it's very easy for one buggy driver to mess up auto-detection for all drivers.
The general rule is that a driver should not set conn->uri to non-NULL until *after* it has determined that it can handle it.
The problem here was that virtualbox driver was setting the uri to non-NULL, and only then seeing if virtalbox was available. My patch fixes that logic ordering bug
Yeah, that seems to fix it for me. It would still be nice to make it a little less error-prone, but I don't have any good ideas at the moment. Thanks! -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel Berrange -
Pritesh Kothari