On 04/16/2012 02:06 PM, Stefan Bader wrote:
> On 16.04.2012 17:21, Cole Robinson wrote:
>> 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@xxxxxxxxxxxxx>
>>>>>>>> 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@xxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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
>
> So the initial failure turned out to be a missing libsasl2-dev. The other two
> are new after the rebase:
>
> TEST: xml2vmxtest
> ...
> 31) VMware XML-2-VMX esx-in-the-wild-2 -> esx-in-the-wild-2
> @@ -7,7 +7,6 @@
> memsize = "1000"
> sched.mem.max = "118"
> numvcpus = "4"
> -sched.cpu.affinity = "0,1,2,5,6,7"
> scsi0.present = "true"
> scsi0.virtualDev = "lsilogic"
> scsi1.present = "true"
> 32) VMware XML-2-VMX esx-in-the-wild-3 -> esx-in-the-wild-3
> @@ -6,7 +6,6 @@
> displayName = "virtDebian2"
> memsize = "1024"
> numvcpus = "2"
> -sched.cpu.affinity = "0,3,4,5"
> scsi0.present = "true"
> scsi0.virtualDev = "lsilogic"
> scsi0:0.present = "true"
> ...
> PASS: test_conf.sh
> --- exp 2012-04-16 17:27:09.672832070 +0000
> +++ out 2012-04-16 17:27:09.672832070 +0000
> @@ -1,3 +1,3 @@
> error: Failed to define domain from xml-invalid
> -error: internal error topology cpuset syntax error
> +error: operation failed: domain 'test' already exists with uuid
> 96b2bf5e-ea49-17b7-ad2c-14308ca3ff59
> FAIL: cpuset
>
> All three go away when I revert the following patch:
>
> commit 8fb2164cfff35ce0b87f1d513a0f3ca5111d7880
> Author: Osier Yang <jyang@xxxxxxxxxx>
> Date: Wed Apr 11 22:40:33 2012 +0800
>
> numad: Ignore cpuset if placement is auto
>
> So I would think I can call my patch tested now. ;)
>
Cool, thanks for checking. ACK to the original patch.
Osier, looks like that patch caused some test fallout?
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list