[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 oracle com>, Joao
Martins <joao m martins oracle com> wrote: 

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

Agree. I'll update the interface.

-Chunyan

>  
> Joao 
>  
>  



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]