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

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?