[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 05/13/2016 03:08 AM, Chun Yan Liu wrote:
> 
> 
>>>> 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.
> 
Indeed.

> (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()).
> 
I am not sure what's the etiquette in these cases but I sent you some patches
that fixes the tests and makes some adjustments to help out (all changelog-ed).
There were a couple of failures lately regarding vram defaults and what not so
some of the tests would fail as vram defaults would be wrongly calculated. Jim
sent a series fixing that which is Acked already but still to be pushed.

Regards,
Joao

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