[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 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?

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.

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