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

[Joao Martins]

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 at 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
these differences, as opposed to a device specific parameter.

Joao