[libvirt] [PATCH 0/2] correct libxl config file convert
Correct libxl config file type=vif handling. Chunyan Liu (2): xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl xlconfigtest: add test case for type=vif in xl format src/xenconfig/xen_common.c | 38 ++++++++++++++++++----------- src/xenconfig/xen_common.h | 7 +++--- src/xenconfig/xen_xl.c | 4 +-- src/xenconfig/xen_xm.c | 8 +++--- tests/xlconfigdata/test-vif-typename.cfg | 25 +++++++++++++++++++ tests/xlconfigdata/test-vif-typename.xml | 42 ++++++++++++++++++++++++++++++++ tests/xlconfigtest.c | 1 + 7 files changed, 102 insertions(+), 23 deletions(-) create mode 100644 tests/xlconfigdata/test-vif-typename.cfg create mode 100644 tests/xlconfigdata/test-vif-typename.xml -- 2.1.4
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront. Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF. Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'. Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg. Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; } - static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup; - if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup; @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) /* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) { if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1; - if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1; if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list, virDomainChrDefPtr serial) return -1; } - static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; } - - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf, for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; } @@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf, /* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1; - if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1; if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf, int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename); int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename); int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1; - if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup; if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup; - if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup; if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1; - if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup; if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup; if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup; if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup; - if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup; if (xenFormatXMOS(conf, def) < 0) -- 2.1.4
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- Spurious newline? I found a couple more, see below. Not sure if it's intended
On 04/21/2016 12:10 PM, Chunyan Liu wrote: this way but except for xenFormatVif() the style seems to consistently leave two newlines for each function.
static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) { if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;
- if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1;
if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list, virDomainChrDefPtr serial) return -1; }
- Here.
static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; }
- Here.
- static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf,
for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; }
@@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf,
/* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1;
- if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1;
if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf,
int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename);
int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - And here.
+ virConnectPtr conn, + const char *vif_typename);
int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup;
if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup;
if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup;
if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; Since you fix the indentation in this function: this line here appears to be missing in this cleanup.
@@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup;
if (xenFormatXMOS(conf, def) < 0)
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename)
One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted.
{ if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;
- if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1;
if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list, virDomainChrDefPtr serial) return -1; }
-
Joao already mentioned the spurious white space changes. My recommendation is to stick with the prevalent pattern in the file.
static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; }
- - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf,
for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; }
@@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf,
/* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1;
- if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1;
if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf,
int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename);
int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename);
int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup;
if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup;
if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup;
if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup;
I think these types of unrelated cleanups should be done in a separate patch. It's a better approach in case someone wants to backport the bug you are fixing here. Regards, Jim
if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup;
if (xenFormatXMOS(conf, def) < 0)
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename)
One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense.
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files?
{ if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;
- if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1;
if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list, virDomainChrDefPtr serial) return -1; }
-
Joao already mentioned the spurious white space changes. My recommendation is to stick with the prevalent pattern in the file.
static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; }
- - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf,
for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; }
@@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf,
/* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1;
- if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1;
if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf,
int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename);
int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename);
int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup;
if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup;
if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup;
if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup;
I think these types of unrelated cleanups should be done in a separate patch. It's a better approach in case someone wants to backport the bug you are fixing here.
Regards, Jim
if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup;
if (xenFormatXMOS(conf, def) < 0)
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL.
Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense.
Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files?
On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g. ... vncunused = 1 vnclisten = "127.0.0.1" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] parallel = "none" serial = "none" builder = "hvm" boot = "d" disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ] to ... vncunused = 1 vnclisten = "127.0.0.1" parallel = "none" serial = "none" builder = "hvm" boot = "d" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ] Regards, Jim
On 05/12/2016 09:55 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL.
Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense.
Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files?
On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g.
Ah, totally missed that out: it looks a large change. I think XL vif won't diverge from XM anytime soon unless we start adding support for more qemu-ish features on xen libxl (e.g. vhostuser, or even block "target" field equivalent). I am fine with the approach on the patch, but the way you suggested is indeed more correct. In case we want to take the hit now and change all tests for this effort I sent Chunyan a patch to fix the tests for inclusion in the next version to help with this effort (also taking in account your recent series to fix the tests). Cheers, Joao
... vncunused = 1 vnclisten = "127.0.0.1" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] parallel = "none" serial = "none" builder = "hvm" boot = "d" disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ]
to
... vncunused = 1 vnclisten = "127.0.0.1" parallel = "none" serial = "none" builder = "hvm" boot = "d" vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ] disk = [ "/dev/HostVG/XenGuest2,raw,hda,rw,backendtype=phy", "/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,rw", "/root/boot.iso,raw,hdc,ro,devtype=cdrom" ]
Regards, Jim
On 05/13/2016 06:59 AM, Joao Martins wrote:
On 05/12/2016 09:55 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense. Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files? On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g.
Ah, totally missed that out: it looks a large change. I think XL vif won't diverge from XM anytime soon unless we start adding support for more qemu-ish features on xen libxl (e.g. vhostuser, or even block "target" field equivalent).
That's a good point. Instead of creating a bunch of turmoil now over 'netfront' vs 'vif', we should wait until something more substantial drives the change.
I am fine with the approach on the patch, but the way you suggested is indeed more correct.
Perhaps as a compromise, the new xen{Format,Parse}ConfigCommon parameter could be of type 'enum xenConfigFlavor' or similar, with flavors XEN_CONFIG_FLAVOR_XL and XEN_CONFIG_FLAVOR_XM. That would accommodate other trivial differences we might find in the future. Regards, Jim
On 05/13/2016 06:59 AM, Joao Martins wrote:
On 05/12/2016 09:55 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense. Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files? On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g.
Ah, totally missed that out: it looks a large change. I think XL vif won't diverge from XM anytime soon unless we start adding support for more qemu-ish features on xen libxl (e.g. vhostuser, or even block "target" field equivalent).
That's a good point. Instead of creating a bunch of turmoil now over 'netfront' vs 'vif', we should wait until something more substantial drives the change.
I am fine with the approach on the patch, but the way you suggested is indeed more correct.
Perhaps as a compromise, the new xen{Format,Parse}ConfigCommon parameter could be of type 'enum xenConfigFlavor' or similar, with flavors XEN_CONFIG_FLAVOR_XL and XEN_CONFIG_FLAVOR_XM. That would accommodate other trivial differences we might find in the future. Yeap 'enum xenConfigFlavor' sounds like a generic enough representation to acommodate
On 05/13/2016 05:54 PM, Jim Fehlig wrote: these differences, as opposed to a device specific parameter. Joao
On 5/16/2016 at 06:14 PM, in message <57399D91.1030307@oracle.com>, Joao Martins <joao.m.martins@oracle.com> wrote:
On 05/13/2016 06:59 AM, Joao Martins wrote:
On 05/12/2016 09:55 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote: > According to current xl.cfg docs and xl codes, it uses type=vif > instead of type=netfront. > > Currently after domxml-to-native, libvirt xml model=netfront will be > converted to xl type=netfront. This has no problem before, xen codes > for a long time just check type=ioemu, if not, set type to _VIF. > > Since libxl uses parse_nic_config to avoid duplicate codes, it > compares 'type=vif' and 'type=ioemu' for valid parameters, others > are considered as invalid, thus we have problem with type=netfront > in xl config file. > #xl create sles12gm-hvm.orig > Parsing config from sles12gm-hvm.orig > Invalid parameter `type'. > > Correct the convertion in libvirt, so that it matchs libxl codes > and also xl.cfg. > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- > src/xenconfig/xen_common.h | 7 ++++--- > src/xenconfig/xen_xl.c | 4 ++-- > src/xenconfig/xen_xm.c | 8 ++++---- > 4 files changed, 34 insertions(+), 23 deletions(-) > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index e1d9cf6..f54d6b6 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) > return -1; > } > > - > static int > -xenParseVif(virConfPtr conf, virDomainDefPtr def) > +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char
*vif_typename)
> { > char *script = NULL; > virDomainNetDefPtr net = NULL; > @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) > VIR_STRDUP(net->model, model) < 0) > goto cleanup; > > - if (!model[0] && type[0] && STREQ(type, "netfront") && > + if (!model[0] && type[0] && STREQ(type, vif_typename) && > VIR_STRDUP(net->model, "netfront") < 0) > goto cleanup; > > @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) > > /* > * A convenience function for parsing all config common to both XM and XL > + * > + * vif_typename: type name for a paravirtualized network could > + * be different for xm and xl. For xm, it uses type=netfront; > + * for xl, it uses type=vif. So, for xm, should pass "netfront"; > + * for xl, should pass "vif". > */ > int > xenParseConfigCommon(virConfPtr conf, > virDomainDefPtr def, > - virCapsPtr caps) > + virCapsPtr caps, > + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that
routine is handling). The vif config is the same between xm and xl, with
small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense. Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all
tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files? On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g.
Ah, totally missed that out: it looks a large change. I think XL vif won't diverge from XM anytime soon unless we start adding support for more qemu-ish features on xen libxl (e.g. vhostuser, or even block "target" field equivalent).
That's a good point. Instead of creating a bunch of turmoil now over 'netfront' vs 'vif', we should wait until something more substantial drives the change.
I am fine with the approach on the patch, but the way you suggested is indeed more correct.
Perhaps as a compromise, the new xen{Format,Parse}ConfigCommon parameter could be of type 'enum xenConfigFlavor' or similar, with flavors XEN_CONFIG_FLAVOR_XL and XEN_CONFIG_FLAVOR_XM. That would accommodate other trivial differences we might find in the future. Yeap 'enum xenConfigFlavor' sounds like a generic enough representation to acommodate
On 05/13/2016 05:54 PM, Jim Fehlig wrote: the the the these differences, as opposed to a device specific parameter.
Agree. I'll update the interface. -Chunyan
Joao
On 5/14/2016 at 12:54 AM, in message <573606AB.4080200@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 05/13/2016 06:59 AM, Joao Martins wrote:
Joao Martins wrote:
On 05/12/2016 12:54 AM, Jim Fehlig wrote:
On 04/21/2016 05:10 AM, Chunyan Liu wrote:
According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename) One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. Nod.
And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that
routine is handling). The vif config is the same between xm and xl, with
small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense. Nod again :-).
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all
On 05/12/2016 09:55 PM, Jim Fehlig wrote: the the the
tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change. Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files? On the Parse side we would be fine, but on the Format side 'vif =' would now be emitted after xenFormatConfigCommon executed. So the xl.cfg output would change from e.g.
Ah, totally missed that out: it looks a large change. I think XL vif won't diverge from XM anytime soon unless we start adding support for more qemu-ish features on xen libxl (e.g. vhostuser, or even block "target" field equivalent).
That's a good point. Instead of creating a bunch of turmoil now over 'netfront' vs 'vif', we should wait until something more substantial drives the change.
I am fine with the approach on the patch, but the way you suggested is indeed more correct.
Perhaps as a compromise, the new xen{Format,Parse}ConfigCommon parameter could be of type 'enum xenConfigFlavor' or similar, with flavors XEN_CONFIG_FLAVOR_XL and XEN_CONFIG_FLAVOR_XM.
We can reuse XEN_CONFIG_FORMAT_XM/XL. Before that we need to unify existing XEN_CONFIG_FORMAT_XM and LIBXL_CONFIG_FORMAT_XM (actually the same) to only one XEN_CONFIG_FORMAT_XM and moved to xen_common.h. - Chunyan
That would accommodate other trivial differences we might find in the future.
Regards, Jim
On 5/12/2016 at 11:00 PM, in message <57349A91.50301@oracle.com>, Joao Martins <joao.m.martins@oracle.com> wrote: On 05/12/2016 12:54 AM, Jim Fehlig wrote: On 04/21/2016 05:10 AM, Chunyan Liu wrote: According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename)
One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense.
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change.
In fact at first I changed codes as Jim suggested, didn't change xenParseConfigCommon(), but extract xen{Format,Parse}Vif out from xenParseConfigCommon() and called from xen_{xl,xm}.c directly. But that will change the order device appears in .cfg or xml. So existing xml and .cfg will fail many times. (I spent quite a bit of time to update xml and .cfg to make all of them correct, still some not pass. That's why finally I updated xenParseConfigCommon()).
Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files?
{ if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;
- if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1;
if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list,
return -1; }
-
Joao already mentioned the spurious white space changes. My recommendation is to stick with the prevalent pattern in the file.
static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; }
- - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf,
for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; }
@@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf,
/* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1;
- if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1;
if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf,
int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename);
int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename);
int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup;
if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup;
if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup;
if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup;
I think these types of unrelated cleanups should be done in a separate
virDomainChrDefPtr serial) patch.
It's a better approach in case someone wants to backport the bug you are fixing here.
Regards, Jim
if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup;
if (xenFormatXMOS(conf, def) < 0)
On 05/13/2016 03:08 AM, Chun Yan Liu wrote:
On 5/12/2016 at 11:00 PM, in message <57349A91.50301@oracle.com>, Joao Martins <joao.m.martins@oracle.com> wrote: On 05/12/2016 12:54 AM, Jim Fehlig wrote: On 04/21/2016 05:10 AM, Chunyan Liu wrote: According to current xl.cfg docs and xl codes, it uses type=vif instead of type=netfront.
Currently after domxml-to-native, libvirt xml model=netfront will be converted to xl type=netfront. This has no problem before, xen codes for a long time just check type=ioemu, if not, set type to _VIF.
Since libxl uses parse_nic_config to avoid duplicate codes, it compares 'type=vif' and 'type=ioemu' for valid parameters, others are considered as invalid, thus we have problem with type=netfront in xl config file. #xl create sles12gm-hvm.orig Parsing config from sles12gm-hvm.orig Invalid parameter `type'.
Correct the convertion in libvirt, so that it matchs libxl codes and also xl.cfg.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/xenconfig/xen_common.c | 38 ++++++++++++++++++++++++-------------- src/xenconfig/xen_common.h | 7 ++++--- src/xenconfig/xen_xl.c | 4 ++-- src/xenconfig/xen_xm.c | 8 ++++---- 4 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index e1d9cf6..f54d6b6 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) return -1; }
- static int -xenParseVif(virConfPtr conf, virDomainDefPtr def) +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { char *script = NULL; virDomainNetDefPtr net = NULL; @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) VIR_STRDUP(net->model, model) < 0) goto cleanup;
- if (!model[0] && type[0] && STREQ(type, "netfront") && + if (!model[0] && type[0] && STREQ(type, vif_typename) && VIR_STRDUP(net->model, "netfront") < 0) goto cleanup;
@@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
/* * A convenience function for parsing all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps) + virCapsPtr caps, + const char *vif_typename)
One thing I didn't recall when suggesting this approach is that xenParseVif() is called in xenParseConfigCommon(). I was thinking it was called from xen_{xl,xm}.c and the extra parameter would only be added to the xen{Format,Parse}Vif functions. I don't particularly like seeing the device specific parameter added to the common functions, but wont object if others are fine with it. Any other opinions on that? Joao? That's a good point - probably we can avoid it by using xen{Format,Parse}Vif (with the signature change Chunyan proposes) individually on xenParseXM and xenParseXL. And there wouldn't be any xenParseConfigCommon with device-specific parameters (as vif being one of the many devices that the routine is handling). The vif config is the same between xm and xl, with the small difference wrt to the validation on xen libxl side - so having in xen_common.c makes sense.
And one reason I wont object is that the alternative (calling xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all the tests/{xl,xm}configdata/ files would need to be adjusted. Hm, perhaps I fail to see what the large change would be. We would keep the same interface (i.e. model=netfront as valid on libvirt-side and converting to type="vif" where applicable (libxl)) then the xml and .cfg won't change.
In fact at first I changed codes as Jim suggested, didn't change xenParseConfigCommon(), but extract xen{Format,Parse}Vif out from xenParseConfigCommon() and called from xen_{xl,xm}.c directly. But that will change the order device appears in .cfg or xml. So existing xml and .cfg will fail many times.
Indeed.
(I spent quite a bit of time to update xml and .cfg to make all of them correct, still some not pass. That's why finally I updated xenParseConfigCommon()).
I am not sure what's the etiquette in these cases but I sent you some patches that fixes the tests and makes some adjustments to help out (all changelog-ed). There were a couple of failures lately regarding vram defaults and what not so some of the tests would fail as vram defaults would be wrongly calculated. Jim sent a series fixing that which is Acked already but still to be pushed. Regards, Joao
Furthermore, we only use e1000 which is valid for both cases and Chunyan adds one test case to cover this series. So may be the adjustment you suggest above wouldn't be as cumbersome as to change all the tests/{xl,xm}configdata files?
{ if (xenParseGeneralMeta(conf, def, caps) < 0) return -1; @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;
- if (xenParseVif(conf, def) < 0) + if (xenParseVif(conf, def, vif_typename) < 0) return -1;
if (xenParsePCI(conf, def) < 0) @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list,
return -1; }
-
Joao already mentioned the spurious white space changes. My recommendation is to stick with the prevalent pattern in the file.
static int xenFormatNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, - int hvm) + int hvm, + const char *vif_typename) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfValuePtr val, tmp; @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, virBufferAsprintf(&buf, ",model=%s", net->model); } else { if (net->model != NULL && STREQ(net->model, "netfront")) { - virBufferAddLit(&buf, ",type=netfront"); + virBufferAsprintf(&buf, ",type=%s", vif_typename); } else { if (net->model != NULL) virBufferAsprintf(&buf, ",model=%s", net->model); @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) return 0; }
- - static int xenFormatVif(virConfPtr conf, virConnectPtr conn, - virDomainDefPtr def) + virDomainDefPtr def, + const char *vif_typename) { virConfValuePtr netVal = NULL; size_t i; @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf,
for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], - hvm) < 0) + hvm, vif_typename) < 0) goto cleanup; }
@@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf,
/* * A convenience function for formatting all config common to both XM and XL + * + * vif_typename: type name for a paravirtualized network could + * be different for xm and xl. For xm, it uses type=netfront; + * for xl, it uses type=vif. So, for xm, should pass "netfront"; + * for xl, should pass "vif". */ int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn) + virConnectPtr conn, + const char *vif_typename) { if (xenFormatGeneralMeta(conf, def) < 0) return -1; @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, if (xenFormatVfb(conf, def) < 0) return -1;
- if (xenFormatVif(conf, conn, def) < 0) + if (xenFormatVif(conf, conn, def, vif_typename) < 0) return -1;
if (xenFormatPCI(conf, def) < 0) diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h index 9ddf210..c1c5fcc 100644 --- a/src/xenconfig/xen_common.h +++ b/src/xenconfig/xen_common.h @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf,
int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, - virCapsPtr caps); + virCapsPtr caps, + const char *vif_typename);
int xenFormatConfigCommon(virConfPtr conf, virDomainDefPtr def, - virConnectPtr conn); - + virConnectPtr conn, + const char *vif_typename);
int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 889dd40..dfd2757 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "vif") < 0) goto cleanup;
if (xenParseXLOS(conf, def, caps) < 0) @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) goto cleanup;
if (xenFormatXLOS(conf, def) < 0) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index e09d97e..5378def 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1;
- if (xenParseConfigCommon(conf, def, caps) < 0) + if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) goto cleanup;
if (xenParseXMOS(conf, def) < 0) - goto cleanup; + goto cleanup;
I think these types of unrelated cleanups should be done in a separate
virDomainChrDefPtr serial) patch.
It's a better approach in case someone wants to backport the bug you are fixing here.
Regards, Jim
if (xenParseXMDisk(conf, def) < 0) - goto cleanup; + goto cleanup;
if (xenParseXMInputDevs(conf, def) < 0) goto cleanup; @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, if (!(conf = virConfNew())) goto cleanup;
- if (xenFormatConfigCommon(conf, def, conn) < 0) + if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) goto cleanup;
if (xenFormatXMOS(conf, def) < 0)
On 05/13/2016 07:08 AM, Joao Martins wrote:
I am not sure what's the etiquette in these cases but I sent you some patches that fixes the tests and makes some adjustments to help out (all changelog-ed). There were a couple of failures lately regarding vram defaults and what not so some of the tests would fail as vram defaults would be wrongly calculated. Jim sent a series fixing that which is Acked already but still to be pushed.
I've pushed those patches now. I think we've finally resolved all the default vram issues. Regards, Jim
On 5/14/2016 at 12:58 AM, in message <5736079B.5050308@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 05/13/2016 07:08 AM, Joao Martins wrote: I am not sure what's the etiquette in these cases but I sent you some patches that fixes the tests and makes some adjustments to help out (all changelog-ed). There were a couple of failures lately regarding vram defaults and what not so some of the tests would fail as vram defaults would be wrongly calculated. Jim sent a series fixing that which is Acked already but still to be pushed.
I've pushed those patches now. I think we've finally resolved all the default vram issues.
Joao, I applied your updated patch series, it looks good. Maybe you can send out to mailing list? Chunyan
Regards, Jim
On 05/16/2016 09:57 AM, Chun Yan Liu wrote:
On 5/14/2016 at 12:58 AM, in message <5736079B.5050308@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 05/13/2016 07:08 AM, Joao Martins wrote: I am not sure what's the etiquette in these cases but I sent you some patches that fixes the tests and makes some adjustments to help out (all changelog-ed). There were a couple of failures lately regarding vram defaults and what not so some of the tests would fail as vram defaults would be wrongly calculated. Jim sent a series fixing that which is Acked already but still to be pushed.
I've pushed those patches now. I think we've finally resolved all the default vram issues.
Joao, I applied your updated patch series, it looks good. Maybe you can send out to mailing list?
Sure! But maybe it's not worth such a big change involving the tests and we should probably wait until something more substantial is required from the vif parsing side [0]. So the initial approach is the one to go - requiring mainly a slight interface change. Jim suggested having an enum to identify which flavor ParseCommon is invoked - which is generic enough representation to handle any trivial disparities between XL and XM. Joao [0] https://www.redhat.com/archives/libvir-list/2016-May/msg01002.html
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- tests/xlconfigdata/test-vif-typename.cfg | 25 +++++++++++++++++++ tests/xlconfigdata/test-vif-typename.xml | 42 ++++++++++++++++++++++++++++++++ tests/xlconfigtest.c | 1 + 3 files changed, 68 insertions(+) create mode 100644 tests/xlconfigdata/test-vif-typename.cfg create mode 100644 tests/xlconfigdata/test-vif-typename.xml diff --git a/tests/xlconfigdata/test-vif-typename.cfg b/tests/xlconfigdata/test-vif-typename.cfg new file mode 100644 index 0000000..2e30777 --- /dev/null +++ b/tests/xlconfigdata/test-vif-typename.cfg @@ -0,0 +1,25 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +pae = 1 +acpi = 1 +apic = 1 +viridian = 0 +rtc_timeoffset = 0 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,type=vif" ] +parallel = "none" +serial = "none" +builder = "hvm" +boot = "d" +disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-vif-typename.xml b/tests/xlconfigdata/test-vif-typename.xml new file mode 100644 index 0000000..1e99377 --- /dev/null +++ b/tests/xlconfigdata/test-vif-typename.xml @@ -0,0 +1,42 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='variable' adjustment='0' basis='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy' type='raw'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <script path='vif-bridge'/> + <model type='netfront'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + </graphics> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index ba1bed0..6819bad 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -228,6 +228,7 @@ mymain(void) DO_TEST_FORMAT("fullvirt-direct-kernel-boot-extra"); DO_TEST_FORMAT("fullvirt-direct-kernel-boot-bogus-extra"); #endif + DO_TEST("vif-typename"); virObjectUnref(caps); virObjectUnref(xmlopt); -- 2.1.4
On 04/21/2016 12:10 PM, Chunyan Liu wrote:
Correct libxl config file type=vif handling.
FWIW, Tested-by: Joao Martins <joao.m.martins@oracle.com> Just detected tiny nitpicks in patch 1, but overall looks good. BTW in testing this I found out a bug in domxml-to-native with xen-xm, so will fix that in a separate patch, as it's unrelated to this series. Regards, Joao
participants (4)
-
Chun Yan Liu -
Chunyan Liu
-
Jim Fehlig -
Joao Martins