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

[Joao Martins]

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).
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
>