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

>> On 5/16/2016 at 06:14 PM, in message <57399D91.1030307(a)oracle.com&gt;, Joao

On 05/13/2016 05:54 PM, Jim Fehlig 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(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. >>> 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 these differences, as opposed to a device specific parameter.

Joao