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.

From a0e214fa6dea9bd66616f37246067a2c631f4184 Mon Sep 17 00:00:00 2001

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

- Cole

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.

The first section of HACKING has some info about things to run before submitting a patch, and ways to debug test failures. Thanks, Cole

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)...

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(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 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(a)redhat.com&gt; 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. ;)

[adding direct cc to Osier] On 04/16/2012 01:58 PM, Cole Robinson wrote: >>>> 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&lt;jyang(a)redhat.com&gt; >> Date: Wed Apr 11 22:40:33 2012 +0800 >> >> numad: Ignore cpuset if placement is auto Yes, I independently hit the same problem and conclusion this morning. >> >> 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?

Daniel and I were discussing it on IRC, although I haven't yet tried patching anything: [09:30] danpb1 eblake: (04:03:19 PM) eblake: +error: operation failed: domain 'test' already exists with uuid 88cb9633-8015-4624-a258-2a3cb9700ad0 [09:31] danpb1 eblake: the fact that we see that error message suggest a pre-existing bug in the test suite - the XML being used to define the test domain is missing a UUID [09:31] danpb1 eblake: so the 2 repeated defines get different auto-generated UUIDs& thus violate the uniqueness check [09:31] danpb1 eblake: i expect if you fixed that problem, then you'd now see the 'define' operation actually suceed

[09:32] eblake but since the test expects a topology syntax error, what changed to make it no longer an error? [09:33] eblake and thus exposing the latent bug in the uuid usage [09:33] danpb1 oh sure, it sounds like we still have a bug in changed semantics [09:34] danpb1 eblake: + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC) { [09:34] danpb1 that line means we now don't ever parse the placement XML and so won't see the bogus data [09:35] danpb1 since the default value is PLACEMENT_MODE_DEFAULT instead of PLACEMENT_MODE_STATIC [09:35] danpb1 we need to treat DEFAULT + STATIC in the same way in the parser [09:36] eblake DEFAULT is primarily useful for where there is a difference in the XML between omitting something and explicitly requesting STATIC [09:36] eblake but if we want to always default to STATIC, maybe we could just trim the enum to get rid of DEFAULT and make STATIC == 0

[09:37] danpb1 i guess the intent was that we don't suddenly fill all existing XML with a new attribute

[09:38] eblake in which case, default (for omitted) vs. static (explicit, don't lose what the user had) makes sense [09:38] eblake but then I agree that we need to handle both modes the same when deciding what else to parse

On 04/16/2012 10:45 PM, Osier Yang wrote: > Two follow up patches, one is to update test read-bufsiz to delete > the UUID, as domain UUID for test driver is fixed now. The other > is to allow the parsing of "cpuset" if the "placement" is not > specified, but "cpuset" is specified, and in this case the "placement" > mode will be set as "static". > > > 0001-conf-Do-not-parse-cpuset-only-if-the-placement-is-au.patch > > > > From 270bb38c25e4fbed32193838dc81ec52a46780c3 Mon Sep 17 00:00:00 2001 > From: Osier Yang&lt;jyang(a)redhat.com&gt; > Date: Tue, 17 Apr 2012 12:40:03 +0800 > Subject: [PATCH] conf: Do not parse cpuset only if the placement is auto > > So that a domain xml which doesn't have "placement" specified, but > "cpuset" is specified, could be parsed. And in this case, the > "placement" mode will be set as "static". > --- > src/conf/domain_conf.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index b28ae5c..65a35c3 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7896,11 +7896,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, > } > VIR_FREE(tmp); > } else { > - if (def->cpumasklen) > - def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; > + def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_DEFAULT; > } > > - if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC) { > + if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { > tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt); > if (tmp) { > char *set = tmp; > @@ -7912,6 +7911,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, > def->cpumasklen)< 0) > goto error; > VIR_FREE(tmp); > + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_DEFAULT) > + def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; ACK, and this fixes the 'make check' failures introduced yesterday. > Subject: [PATCH] tests: Update read-bufsiz to delete the UUID of vm XML > > Since now we have fixed domain UUID for test driver, defining > a domain with different name but same UUID doesn't work any > more. This patch delete the UUID from the dumped XML so that > it could be generated. > --- > tests/read-bufsiz | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/read-bufsiz b/tests/read-bufsiz > index 2a91bcf..a4c6007 100755 > --- a/tests/read-bufsiz > +++ b/tests/read-bufsiz > @@ -32,8 +32,10 @@ fail=0 > # Output a valid definition, to be used as input. > $abs_top_builddir/tools/virsh -c test:///default dumpxml 1> xml.t || fail=1 > > -# Change the VM name > -sed -e "s|<name>test</name>|<name>newtest</name>|g" xml.t> xml > +# Change the VM name and UUID > +sed -e "s|<name>test</name>|<name>newtest</name>|g" \ > + -e "\|<uuid>.*</uuid>|d" \ > + xml.t> xml ACK, and this fixes the problems introduced with the hard-coded UUID. Please apply.