On Wed, Sep 09, 2015 at 03:30:07PM +0200, Martin Kletzander wrote:
On Wed, Sep 09, 2015 at 02:50:23PM +0200, Andrea Bolognani wrote:Up until now, the default has been rtl8139, but no check was in place to make sure that device was actually available. Now we try rtl8139, e1000 and virtio-net in turn, checking for availability before using any of them: this means we have a much better chance for the guest to be able to boot. --- src/qemu/qemu_domain.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 91c632c..33cd463 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1193,7 +1193,8 @@ qemuDomainDefPostParse(virDomainDefPtr def, } static const char * -qemuDomainDefaultNetModel(const virDomainDef *def) +qemuDomainDefaultNetModel(const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { if (ARCH_IS_S390(def->os.arch)) return "virtio"; @@ -1211,7 +1212,23 @@ qemuDomainDefaultNetModel(const virDomainDef *def) return "lan9118"; } - return "rtl8139"; + /* Try several network devices in turn; each of these devices is + * less likely be supported out-of-the-box by the guest operating + * system than the previous one. + * + * Note: Using rtl8139 as a last resort, even though by the time + * we get there we already know that it's not available, is an + * ugly hack needed to work around the fact that we don't have + * a way to mock capabilities in the test suite yet. Once we do, + * we should return NULL and raise an error instead */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) + return "rtl8139"; + else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) + return "e1000"; + else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) + return "virtio"; + elseI would remove the comment, remove this "else" and just comment the 'return "rtl8139"' with something along the lines of "we have nothing else to do here, so we can return an error, but rtl8139 may still be supported if the capability probing was broken in any way, so let's use it as a last resort (the same way we did before). Also having a test for the thing you are trying to fix would be nice. Otherwise this code looks fine.
I missed the fact that our test suite is not yet prepared to do that, sorry for that. So I'd say ACK with the previous paragraph being addressed.
+ return "rtl8139"; } static int @@ -1220,18 +1237,24 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) { - int ret = -1; virQEMUDriverPtr driver = opaque; + virQEMUCapsPtr qemuCaps = NULL; virQEMUDriverConfigPtr cfg = NULL; + int ret = -1; if (driver) cfg = virQEMUDriverGetConfig(driver); + /* This condition is actually a (temporary) hack for test suite which + * does not create capabilities cache */ + if (driver && driver->qemuCapsCache) + qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator); + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !dev->data.net->model) { if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def)) < 0) + qemuDomainDefaultNetModel(def, qemuCaps)) < 0) goto cleanup; } @@ -1357,6 +1380,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, ret = 0; cleanup: + virObjectUnref(qemuCaps); virObjectUnref(cfg); return ret; } -- 2.4.3 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list