Re: [libvirt] [PATCH v2] libvirt: xen: do not use ioemu type for any emulated NIC

On 04/16/2012 10:11 AM, Stefan Bader wrote: > On 16.04.2012 14:54, Stefan Bader wrote: >> On 16.04.2012 14:45, Cole Robinson wrote: >>> On 04/16/2012 08:36 AM, Stefan Bader wrote: >>>> On 16.04.2012 13:58, Cole Robinson wrote: >>>>> On 04/13/2012 09:14 AM, Stefan Bader wrote: >>>>>>> I think it would be better if we just centralized this logic, as in, >>>>>>> only set that (type ioemu) bit in conditional rather than 2. Should >>>>>>> be pretty straightforward. >>>>>> >>>>>> Did you have something like below in mind? >>>>>> >>>>>> -Stefan >>>>>> >>>>>> >From a0e214fa6dea9bd66616f37246067a2c631f4184 Mon Sep 17 00:00:00 2001 >>>>>> From: Stefan Bader <stefan.bader(a)canonical.com&gt; >>>>>> Date: Thu, 12 Apr 2012 15:32:41 +0200 >>>>>> Subject: [PATCH] libvirt: xen: do not use ioemu type for any emulated NIC >>>>>> >>>>>> When using the xm/xend stack to manage instances there is a bug >>>>>> that causes the emulated interfaces to be unusable when the vif >>>>>> config contains type=ioemu. >>>>>> >>>>>> The current code already has a special quirk to not use this >>>>>> keyword if no specific model is given for the emulated NIC >>>>>> (defaulting to rtl8139). >>>>>> Essentially it works because regardless of the type argument,i >>>>>> the Xen stack always creates emulated and paravirt interfaces and >>>>>> lets the guest decide which one to use. So neither xl nor xm stack >>>>>> actually require the type keyword for emulated NICs. >>>>>> >>>>>> [v2: move code around to have the exception in one case together] >>>>>> Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; >>>>>> --- >>>>>> src/xenxs/xen_sxpr.c | 28 +++++++++++++++------------- >>>>>> src/xenxs/xen_xm.c | 27 ++++++++++++++------------- >>>>>> 2 files changed, 29 insertions(+), 26 deletions(-) >>>>>> >>>>>> diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c >>>>>> index e1bbd76..3a56345 100644 >>>>>> --- a/src/xenxs/xen_sxpr.c >>>>>> +++ b/src/xenxs/xen_sxpr.c >>>>>> @@ -1999,20 +1999,22 @@ xenFormatSxprNet(virConnectPtr conn, >>>>>> if (def->model != NULL) >>>>>> virBufferEscapeSexpr(buf, "(model '%s')", def->model); >>>>>> } >>>>>> - else if (def->model == NULL) { >>>>>> - /* >>>>>> - * apparently (type ioemu) breaks paravirt drivers on HVM so skip >>>>>> - * this from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >>>>>> - */ >>>>>> - if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >>>>>> - virBufferAddLit(buf, "(type ioemu)"); >>>>>> - } >>>>>> - else if (STREQ(def->model, "netfront")) { >>>>>> - virBufferAddLit(buf, "(type netfront)"); >>>>>> - } >>>>>> else { >>>>>> - virBufferEscapeSexpr(buf, "(model '%s')", def->model); >>>>>> - virBufferAddLit(buf, "(type ioemu)"); >>>>>> + if (def->model != NULL && STREQ(def->model, "netfront")) { >>>>>> + virBufferAddLit(buf, "(type netfront)"); >>>>>> + } >>>>>> + else { >>>>>> + if (def->model != NULL) { >>>>>> + virBufferEscapeSexpr(buf, "(model '%s')", def->model); >>>>>> + } >>>>>> + /* >>>>>> + * apparently (type ioemu) breaks paravirt drivers on HVM so skip >>>>>> + * this from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >>>>>> + */ >>>>>> + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) { >>>>>> + virBufferAddLit(buf, "(type ioemu)"); >>>>>> + } >>>>>> + } >>>>>> } >>>>>> >>>>>> if (!isAttach) >>>>>> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c >>>>>> index d65e97a..93a26f9 100644 >>>>>> --- a/src/xenxs/xen_xm.c >>>>>> +++ b/src/xenxs/xen_xm.c >>>>>> @@ -1381,20 +1381,21 @@ static int xenFormatXMNet(virConnectPtr conn, >>>>>> if (net->model != NULL) >>>>>> virBufferAsprintf(&buf, ",model=%s", net->model); >>>>>> } >>>>>> - else if (net->model == NULL) { >>>>>> - /* >>>>>> - * apparently type ioemu breaks paravirt drivers on HVM so skip this >>>>>> - * from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >>>>>> - */ >>>>>> - if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >>>>>> - virBufferAddLit(&buf, ",type=ioemu"); >>>>>> - } >>>>>> - else if (STREQ(net->model, "netfront")) { >>>>>> - virBufferAddLit(&buf, ",type=netfront"); >>>>>> - } >>>>>> else { >>>>>> - virBufferAsprintf(&buf, ",model=%s", net->model); >>>>>> - virBufferAddLit(&buf, ",type=ioemu"); >>>>>> + if (net->model != NULL && STREQ(net->model, "netfront")) { >>>>>> + virBufferAddLit(&buf, ",type=netfront"); >>>>>> + } >>>>>> + else { >>>>>> + if (net->model != NULL) >>>>>> + virBufferAsprintf(&buf, ",model=%s", net->model); >>>>>> + >>>>>> + /* >>>>>> + * apparently type ioemu breaks paravirt drivers on HVM so skip this >>>>>> + * from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >>>>>> + */ >>>>>> + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >>>>>> + virBufferAddLit(&buf, ",type=ioemu"); >>>>>> + } >>>>>> } >>>>>> >>>>>> if (net->ifname) >>>>> >>>>> Looks good. Did you verify 'make check' passes as well? >>>>> >>>>> If so, ACK >>>> >>>> No :( And it fails. >>>> >>>> TEST: libvirtdconftest >>>> .....!!!!!!...!!!!!!!!!!!!!!!!!!!!!!! 37 FAIL >>>> FAIL: libvirtdconftest >>>> >>>> I guess this is expected as the change really does modify the generated configs. >>>> So I need to find out how to make it expect that... >>>> >>> >>> Make sure it's actually your patch causing those issues. I wouldn't expect >>> your change to affect that particular test. >>> >> Right, I just ran the checks with my patch reverted and it fails exactly the >> same way. So I'll rebase to todays HEAD to see whether that is better. >> But basically you are right, my patch does not change anything. >> > > Rebasing to HEAD made things rather worse... now 3 checks are failing (without > my patch)... > Might be something simple like missing a build dep. Try getting some debug output from the tests and it might be clear. If it isn't something simple, I can give your patch a spin. - Cole