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

[Cole Robinson]

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 at canonical.com>
>>>>>> 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 at canonical.com>
>>>>>> ---
>>>>>>  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