Re: [libvirt] [PATCH 1/2] xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl

>> On 5/12/2016 at 11:00 PM, in message <57349A91.50301(a)oracle.com&gt;, Joao Martins

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