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

[Jim Fehlig]

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.

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