[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/12/2016 at 11:00 PM, in message <57349A91 50301 oracle com>, Joao Martins
<joao m martins oracle com> 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. 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. 
>  
> > 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.  

In fact at first I changed codes as Jim suggested, didn't change 
xenParseConfigCommon(), but extract xen{Format,Parse}Vif out from
xenParseConfigCommon() and called from xen_{xl,xm}.c directly. But that
will change the order device appears in .cfg or xml. So existing xml and
.cfg will fail many times.

(I spent quite a bit of time to update xml and .cfg to make all of them correct,
 still some not pass. That's why finally I updated xenParseConfigCommon()).

> 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? 
>  
> >  
> >>  { 
> >>      if (xenParseGeneralMeta(conf, def, caps) < 0) 
> >>          return -1; 
> >> @@ -1066,7 +1071,7 @@ xenParseConfigCommon(virConfPtr conf, 
> >>      if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) 
> >>          return -1; 
> >>   
> >> -    if (xenParseVif(conf, def) < 0) 
> >> +    if (xenParseVif(conf, def, vif_typename) < 0) 
> >>          return -1; 
> >>   
> >>      if (xenParsePCI(conf, def) < 0) 
> >> @@ -1122,12 +1127,12 @@ xenFormatSerial(virConfValuePtr list,  
> virDomainChrDefPtr serial) 
> >>      return -1; 
> >>  } 
> >>   
> >> - 
> >  
> > Joao already mentioned the spurious white space changes. My recommendation  
> is to 
> > stick with the prevalent pattern in the file. 
> >  
> >>  static int 
> >>  xenFormatNet(virConnectPtr conn, 
> >>               virConfValuePtr list, 
> >>               virDomainNetDefPtr net, 
> >> -             int hvm) 
> >> +             int hvm, 
> >> +             const char *vif_typename) 
> >>  { 
> >>      virBuffer buf = VIR_BUFFER_INITIALIZER; 
> >>      virConfValuePtr val, tmp; 
> >> @@ -1199,7 +1204,7 @@ xenFormatNet(virConnectPtr conn, 
> >>              virBufferAsprintf(&buf, ",model=%s", net->model); 
> >>      } else { 
> >>          if (net->model != NULL && STREQ(net->model, "netfront")) { 
> >> -            virBufferAddLit(&buf, ",type=netfront"); 
> >> +            virBufferAsprintf(&buf, ",type=%s", vif_typename); 
> >>          } else { 
> >>              if (net->model != NULL) 
> >>                  virBufferAsprintf(&buf, ",model=%s", net->model); 
> >> @@ -1744,12 +1749,11 @@ xenFormatSound(virConfPtr conf, virDomainDefPtr def) 
> >>      return 0; 
> >>  } 
> >>   
> >> - 
> >> - 
> >>  static int 
> >>  xenFormatVif(virConfPtr conf, 
> >>               virConnectPtr conn, 
> >> -             virDomainDefPtr def) 
> >> +             virDomainDefPtr def, 
> >> +             const char *vif_typename) 
> >>  { 
> >>     virConfValuePtr netVal = NULL; 
> >>     size_t i; 
> >> @@ -1762,7 +1766,7 @@ xenFormatVif(virConfPtr conf, 
> >>   
> >>      for (i = 0; i < def->nnets; i++) { 
> >>          if (xenFormatNet(conn, netVal, def->nets[i], 
> >> -                         hvm) < 0) 
> >> +                         hvm, vif_typename) < 0) 
> >>             goto cleanup; 
> >>      } 
> >>   
> >> @@ -1784,11 +1788,17 @@ xenFormatVif(virConfPtr conf, 
> >>   
> >>  /* 
> >>   * A convenience function for formatting 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 
> >>  xenFormatConfigCommon(virConfPtr conf, 
> >>                        virDomainDefPtr def, 
> >> -                      virConnectPtr conn) 
> >> +                      virConnectPtr conn, 
> >> +                      const char *vif_typename) 
> >>  { 
> >>      if (xenFormatGeneralMeta(conf, def) < 0) 
> >>          return -1; 
> >> @@ -1814,7 +1824,7 @@ xenFormatConfigCommon(virConfPtr conf, 
> >>      if (xenFormatVfb(conf, def) < 0) 
> >>          return -1; 
> >>   
> >> -    if (xenFormatVif(conf, conn, def) < 0) 
> >> +    if (xenFormatVif(conf, conn, def, vif_typename) < 0) 
> >>          return -1; 
> >>   
> >>      if (xenFormatPCI(conf, def) < 0) 
> >> diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h 
> >> index 9ddf210..c1c5fcc 100644 
> >> --- a/src/xenconfig/xen_common.h 
> >> +++ b/src/xenconfig/xen_common.h 
> >> @@ -54,12 +54,13 @@ int xenConfigCopyStringOpt(virConfPtr conf, 
> >>   
> >>  int xenParseConfigCommon(virConfPtr conf, 
> >>                           virDomainDefPtr def, 
> >> -                         virCapsPtr caps); 
> >> +                         virCapsPtr caps, 
> >> +                         const char *vif_typename); 
> >>   
> >>  int xenFormatConfigCommon(virConfPtr conf, 
> >>                            virDomainDefPtr def, 
> >> -                          virConnectPtr conn); 
> >> - 
> >> +                          virConnectPtr conn, 
> >> +                          const char *vif_typename); 
> >>   
> >>  int xenDomainDefAddImplicitInputDevice(virDomainDefPtr def); 
> >>   
> >> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c 
> >> index 889dd40..dfd2757 100644 
> >> --- a/src/xenconfig/xen_xl.c 
> >> +++ b/src/xenconfig/xen_xl.c 
> >> @@ -499,7 +499,7 @@ xenParseXL(virConfPtr conf, 
> >>      def->virtType = VIR_DOMAIN_VIRT_XEN; 
> >>      def->id = -1; 
> >>   
> >> -    if (xenParseConfigCommon(conf, def, caps) < 0) 
> >> +    if (xenParseConfigCommon(conf, def, caps, "vif") < 0) 
> >>          goto cleanup; 
> >>   
> >>      if (xenParseXLOS(conf, def, caps) < 0) 
> >> @@ -994,7 +994,7 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) 
> >>      if (!(conf = virConfNew())) 
> >>          goto cleanup; 
> >>   
> >> -    if (xenFormatConfigCommon(conf, def, conn) < 0) 
> >> +    if (xenFormatConfigCommon(conf, def, conn, "vif") < 0) 
> >>          goto cleanup; 
> >>   
> >>      if (xenFormatXLOS(conf, def) < 0) 
> >> diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c 
> >> index e09d97e..5378def 100644 
> >> --- a/src/xenconfig/xen_xm.c 
> >> +++ b/src/xenconfig/xen_xm.c 
> >> @@ -443,14 +443,14 @@ xenParseXM(virConfPtr conf, 
> >>      def->virtType = VIR_DOMAIN_VIRT_XEN; 
> >>      def->id = -1; 
> >>   
> >> -    if (xenParseConfigCommon(conf, def, caps) < 0) 
> >> +    if (xenParseConfigCommon(conf, def, caps, "netfront") < 0) 
> >>          goto cleanup; 
> >>   
> >>      if (xenParseXMOS(conf, def) < 0) 
> >> -         goto cleanup; 
> >> +        goto cleanup; 
> >  
> > I think these types of unrelated cleanups should be done in a separate  
> patch. 
> > It's a better approach in case someone wants to backport the bug you are  
> fixing 
> > here. 
> >  
> > Regards, 
> > Jim 
> >  
> >>   
> >>      if (xenParseXMDisk(conf, def) < 0) 
> >> -         goto cleanup; 
> >> +        goto cleanup; 
> >>   
> >>      if (xenParseXMInputDevs(conf, def) < 0) 
> >>           goto cleanup; 
> >> @@ -586,7 +586,7 @@ xenFormatXM(virConnectPtr conn, 
> >>      if (!(conf = virConfNew())) 
> >>          goto cleanup; 
> >>   
> >> -    if (xenFormatConfigCommon(conf, def, conn) < 0) 
> >> +    if (xenFormatConfigCommon(conf, def, conn, "netfront") < 0) 
> >>          goto cleanup; 
> >>   
> >>      if (xenFormatXMOS(conf, def) < 0) 
> >  
>  
>  



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