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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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.

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

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list