[libvirt] [PATCH v3 4/4] qemu: Try several network devices when looking for a default

[Martin Kletzander]

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";
>>+    else
>
>I 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 at redhat.com
>>https://www.redhat.com/mailman/listinfo/libvir-list



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150909/36abb830/attachment-0001.sig>