[libvirt] [PATCH] libvirt: xen: do not use ioemu type for any emulated NIC
This is a re-send as there was some positive feedback but the patch itself made it into the repo. -Stefan
From a3198c5c1ae8908818f6c0f0df4237dbe5ddeec7 Mon Sep 17 00:00:00 2001 From: Stefan Bader <stefan.bader@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 (MAC address all zero). 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, 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. Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/xenxs/xen_sxpr.c | 4 +++- src/xenxs/xen_xm.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index e1bbd76..71602fa 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2012,7 +2012,9 @@ xenFormatSxprNet(virConnectPtr conn, } else { virBufferEscapeSexpr(buf, "(model '%s')", def->model); - virBufferAddLit(buf, "(type ioemu)"); + /* See above. Also needed when model is specified. */ + 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..e072599 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1394,7 +1394,9 @@ static int xenFormatXMNet(virConnectPtr conn, } else { virBufferAsprintf(&buf, ",model=%s", net->model); - virBufferAddLit(&buf, ",type=ioemu"); + /* See above. Also needed if model is specified. */ + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) + virBufferAddLit(&buf, ",type=ioemu"); } if (net->ifname) -- 1.7.9.5
On 04/12/2012 09:42 AM, Stefan Bader wrote:
This is a re-send as there was some positive feedback but the patch itself made it into the repo.
-Stefan
From a3198c5c1ae8908818f6c0f0df4237dbe5ddeec7 Mon Sep 17 00:00:00 2001 From: Stefan Bader <stefan.bader@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 (MAC address all zero).
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, 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.
Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- src/xenxs/xen_sxpr.c | 4 +++- src/xenxs/xen_xm.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index e1bbd76..71602fa 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2012,7 +2012,9 @@ xenFormatSxprNet(virConnectPtr conn, } else { virBufferEscapeSexpr(buf, "(model '%s')", def->model); - virBufferAddLit(buf, "(type ioemu)"); + /* See above. Also needed when model is specified. */ + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) + virBufferAddLit(buf, "(type ioemu)"); }
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.
if (!isAttach) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index d65e97a..e072599 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1394,7 +1394,9 @@ static int xenFormatXMNet(virConnectPtr conn, } else { virBufferAsprintf(&buf, ",model=%s", net->model); - virBufferAddLit(&buf, ",type=ioemu"); + /* See above. Also needed if model is specified. */ + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) + virBufferAddLit(&buf, ",type=ioemu"); }
if (net->ifname)
Same here as well. Thanks, Cole
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@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@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) -- 1.7.9.5
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@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@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 - Cole
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@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@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...
- 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@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@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. 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: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@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@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. -Stefan
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@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@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)... -Stefan
-Stefan
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 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@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@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
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@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@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
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@redhat.com> 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. ;) -Stefan
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@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@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
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@redhat.com> 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
[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 <jyang@redhat.com> 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 -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年04月17日 04:17, Eric Blake wrote:
[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<jyang@redhat.com> 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?
Oops, yes.
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
Not true, there is UUID in the dumped xml: # for i in {1..3}; do ./tools/virsh --connect test:///default \ dumpxml test | grep uuid; done <uuid>a1b6ee1f-97de-f0ee-617a-0cdb74947df5</uuid> <uuid>ee68d7d2-3eb9-593e-2769-797ce1f4c4aa</uuid> <uuid>fecb1d3a-918a-8412-e534-76192cf32b18</uuid> It outputs different UUID each time. I'm going to fix this first.
[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
Yes, that was the intent, "static" when "cpuset" is specified manually, and "default" for "cpuset" is not specified.
[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
I will post a patch soon after the uuid problem of test driver, to treat "default" and "static" the same (not to parse cpuset). Regards, Osier
On 04/16/2012 08:05 PM, Osier Yang wrote:
[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
Not true, there is UUID in the dumped xml:
A generated UUID - that is, we have a latent bug, in that since we aren't locking down a specific UUID, the generated one is different each time, and the test failed (as expected), but for the wrong reason (mismatch in UUID, instead of the intended failure due to a cpuset syntax error). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年04月17日 10:44, Eric Blake wrote:
On 04/16/2012 08:05 PM, Osier Yang wrote:
[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
Not true, there is UUID in the dumped xml:
A generated UUID - that is, we have a latent bug, in that since we aren't locking down a specific UUID, the generated one is different each time, and the test failed (as expected), but for the wrong reason (mismatch in UUID, instead of the intended failure due to a cpuset syntax error).
Yeah, just because the cpuset syntax error comes up earlier before the UUID checking. I post a patch to set the fixed UUID for the objects which support UUID in test driver, see the attachment. Regards, Osier
On 04/16/2012 09:03 PM, Osier Yang wrote:
On 2012年04月17日 10:44, Eric Blake wrote:
On 04/16/2012 08:05 PM, Osier Yang wrote:
[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
Not true, there is UUID in the dumped xml:
A generated UUID - that is, we have a latent bug, in that since we aren't locking down a specific UUID, the generated one is different each time, and the test failed (as expected), but for the wrong reason (mismatch in UUID, instead of the intended failure due to a cpuset syntax error).
Yeah, just because the cpuset syntax error comes up earlier before the UUID checking. I post a patch to set the fixed UUID for the objects which support UUID in test driver, see the attachment.
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年04月17日 11:38, Eric Blake wrote:
On 04/16/2012 09:03 PM, Osier Yang wrote:
On 2012年04月17日 10:44, Eric Blake wrote:
On 04/16/2012 08:05 PM, Osier Yang wrote:
[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
Not true, there is UUID in the dumped xml:
A generated UUID - that is, we have a latent bug, in that since we aren't locking down a specific UUID, the generated one is different each time, and the test failed (as expected), but for the wrong reason (mismatch in UUID, instead of the intended failure due to a cpuset syntax error).
Yeah, just because the cpuset syntax error comes up earlier before the UUID checking. I post a patch to set the fixed UUID for the objects which support UUID in test driver, see the attachment.
ACK.
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".
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 <jyang@redhat.com> 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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年04月17日 22:49, Eric Blake wrote:
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<jyang@redhat.com> 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.
Thanks, pushed all the 3 patches. Regards, Osier
On 04/16/2012 05:58 AM, 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?
Looks good. Did you verify 'make check' passes as well?
If so, ACK
Pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Cole Robinson -
Eric Blake -
Osier Yang -
Stefan Bader