Re: [PATCH LIBVIRT v1 1/2] libxl: Correct value for xendConfigVersion to xen{Parse, Format}ConfigCommon

[Jim Fehlig <jfehlig@suse.com>]

On 11/26/2015 09:59 AM, Ian Campbell wrote:
> libxlConnectDomainXMLFromNative calls both xenParseXM and xenParseXL
> with cfg->verInfo->xen_version_major, however AFAICT they both (either
> inherently, or through there use of xenParseConfigCommon expect a
> value from xenConfigVersionEnum (which does not correspond to
> xen_version_major).

I recall being a little apprehensive about using xen_version_major when writing
the code, and apparently that was justified. But now that we are revisiting
this, I wonder if we care about these old xend config versions at all. Is anyone
even running Xen 3.0.x, or 3.1.x, particularly with a newish libvirt? IMO we
could drop all of this xend config nonsense and go with the code associated with
the last xend config version, i.e. XEND_CONFIG_VERSION_3_1_0.

And while mentioning dropping support for these old xend config versions, do I
dare mention dropping the xend driver altogether? :-) It will certainly need to
be done some day. Question is whether that's a bit premature now.

Regards,
Jim

>
> The actual xend backend passes priv->xendConfigVersion, which seems to
> have been gotten from xend and I assume conforms to that enum, via the
> node/xend_config_format value in the xend sxp.
>
> Add a new value to that enum, XEND_CONFIG_VERSION_4_0_0, on the basis
> that the xl syntax was originally based on (and intended to be
> compatible with) xm circa that point in time and that I will shortly
> want to add handling of a cfg file difference which occured in xend in
> 4.0.0 (maxvcpus instead of vpu_avail bitmask).
>
> Pass this from every caller of xen{Parse,Format}X? in the libxl
> driver.
>
> Update xlconfigtest to pass this new value, and regenerate the output
> files with that (all of the changes are expected AFAICT).
>
> The enum and the parameters are already slightly misnamed now (since
> they kind-of apply to xl too), but I didn't go through and rename
> them. In the future we may want to add new values to the enum to
> reflect evolution of the xl cfg syntax (xend was essentially static
> from 4.0 until it was removed), at that point renaming should probably
> be considered.
>
> Note also that xend's xend_config_format remained at 4 until it's
> removal in Xen 4.5, so there will be no actual change in behaviour for
> xm/xend here.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  src/libxl/libxl_driver.c                                |  8 ++++----
>  src/xenconfig/xen_sxpr.h                                |  1 +
>  tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg |  3 ++-
>  tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml |  2 +-
>  tests/xlconfigdata/test-fullvirt-multiusb.cfg           |  3 ++-
>  tests/xlconfigdata/test-fullvirt-multiusb.xml           |  2 +-
>  tests/xlconfigdata/test-new-disk.cfg                    |  3 ++-
>  tests/xlconfigdata/test-new-disk.xml                    |  2 +-
>  tests/xlconfigdata/test-spice-features.cfg              |  3 ++-
>  tests/xlconfigdata/test-spice-features.xml              |  2 +-
>  tests/xlconfigdata/test-spice.cfg                       |  3 ++-
>  tests/xlconfigdata/test-spice.xml                       |  2 +-
>  tests/xlconfigtest.c                                    | 10 +++++-----
>  13 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 35d7fae..892ae44 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2587,14 +2587,14 @@ libxlConnectDomainXMLFromNative(virConnectPtr conn,
>              goto cleanup;
>          if (!(def = xenParseXL(conf,
>                                 cfg->caps,
> -                               cfg->verInfo->xen_version_major)))
> +                               XEND_CONFIG_VERSION_4_0_0)))
>              goto cleanup;
>      } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
>          if (!(conf = virConfReadMem(nativeConfig, strlen(nativeConfig), 0)))
>              goto cleanup;
>  
>          if (!(def = xenParseXM(conf,
> -                               cfg->verInfo->xen_version_major,
> +                               XEND_CONFIG_VERSION_4_0_0,
>                                 cfg->caps)))
>              goto cleanup;
>      } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_SEXPR)) {
> @@ -2647,10 +2647,10 @@ libxlConnectDomainXMLToNative(virConnectPtr conn, const char * nativeFormat,
>          goto cleanup;
>  
>      if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XL)) {
> -        if (!(conf = xenFormatXL(def, conn, cfg->verInfo->xen_version_major)))
> +        if (!(conf = xenFormatXL(def, conn, XEND_CONFIG_VERSION_4_0_0)))
>              goto cleanup;
>      } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
> -        if (!(conf = xenFormatXM(conn, def, cfg->verInfo->xen_version_major)))
> +        if (!(conf = xenFormatXM(conn, def, XEND_CONFIG_VERSION_4_0_0)))
>              goto cleanup;
>      } else {
>  
> diff --git a/src/xenconfig/xen_sxpr.h b/src/xenconfig/xen_sxpr.h
> index f354a50..bb9ed3c 100644
> --- a/src/xenconfig/xen_sxpr.h
> +++ b/src/xenconfig/xen_sxpr.h
> @@ -37,6 +37,7 @@ typedef enum {
>      XEND_CONFIG_VERSION_3_0_3 = 2,
>      XEND_CONFIG_VERSION_3_0_4 = 3,
>      XEND_CONFIG_VERSION_3_1_0 = 4,
> +    XEND_CONFIG_VERSION_4_0_0 = 5,
>  } xenConfigVersionEnum;
>  
>  /* helper functions to get the dom id from a sexpr */
> diff --git a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
> index 1fac3a5..f452af6 100644
> --- a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
> +++ b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
> @@ -8,6 +8,7 @@ acpi = 1
>  apic = 1
>  hap = 0
>  viridian = 0
> +rtc_timeoffset = 0
>  localtime = 0
>  on_poweroff = "destroy"
>  on_reboot = "restart"
> @@ -18,7 +19,7 @@ vnc = 1
>  vncunused = 1
>  vnclisten = "127.0.0.1"
>  vncpasswd = "123poi"
> -vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>  parallel = "none"
>  serial = "none"
>  builder = "hvm"
> diff --git a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml
> index 414f645..5298d19 100644
> --- a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml
> +++ b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml
> @@ -17,7 +17,7 @@
>      <apic/>
>      <pae/>
>    </features>
> -  <clock offset='utc' adjustment='reset'/>
> +  <clock offset='variable' adjustment='0' basis='utc'/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>restart</on_crash>
> diff --git a/tests/xlconfigdata/test-fullvirt-multiusb.cfg b/tests/xlconfigdata/test-fullvirt-multiusb.cfg
> index 68a2614..d0482a8 100755
> --- a/tests/xlconfigdata/test-fullvirt-multiusb.cfg
> +++ b/tests/xlconfigdata/test-fullvirt-multiusb.cfg
> @@ -8,6 +8,7 @@ acpi = 1
>  apic = 1
>  hap = 0
>  viridian = 0
> +rtc_timeoffset = 0
>  localtime = 0
>  on_poweroff = "destroy"
>  on_reboot = "restart"
> @@ -18,7 +19,7 @@ vnc = 1
>  vncunused = 1
>  vnclisten = "127.0.0.1"
>  vncpasswd = "123poi"
> -vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>  parallel = "none"
>  serial = "none"
>  builder = "hvm"
> diff --git a/tests/xlconfigdata/test-fullvirt-multiusb.xml b/tests/xlconfigdata/test-fullvirt-multiusb.xml
> index 642c242..7331613 100644
> --- a/tests/xlconfigdata/test-fullvirt-multiusb.xml
> +++ b/tests/xlconfigdata/test-fullvirt-multiusb.xml
> @@ -14,7 +14,7 @@
>      <apic/>
>      <pae/>
>    </features>
> -  <clock offset='utc' adjustment='reset'/>
> +  <clock offset='variable' adjustment='0' basis='utc'/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>restart</on_crash>
> diff --git a/tests/xlconfigdata/test-new-disk.cfg b/tests/xlconfigdata/test-new-disk.cfg
> index 9e9f106..9b9fb36 100644
> --- a/tests/xlconfigdata/test-new-disk.cfg
> +++ b/tests/xlconfigdata/test-new-disk.cfg
> @@ -8,6 +8,7 @@ acpi = 1
>  apic = 1
>  hap = 0
>  viridian = 0
> +rtc_timeoffset = 0
>  localtime = 0
>  on_poweroff = "destroy"
>  on_reboot = "restart"
> @@ -17,7 +18,7 @@ sdl = 0
>  vnc = 1
>  vncunused = 1
>  vnclisten = "127.0.0.1"
> -vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>  parallel = "none"
>  serial = "none"
>  builder = "hvm"
> diff --git a/tests/xlconfigdata/test-new-disk.xml b/tests/xlconfigdata/test-new-disk.xml
> index 1c96a62..7a74926 100644
> --- a/tests/xlconfigdata/test-new-disk.xml
> +++ b/tests/xlconfigdata/test-new-disk.xml
> @@ -14,7 +14,7 @@
>      <apic/>
>      <pae/>
>    </features>
> -  <clock offset='utc' adjustment='reset'/>
> +  <clock offset='variable' adjustment='0' basis='utc'/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>restart</on_crash>
> diff --git a/tests/xlconfigdata/test-spice-features.cfg b/tests/xlconfigdata/test-spice-features.cfg
> index c3e7111..152cb27 100644
> --- a/tests/xlconfigdata/test-spice-features.cfg
> +++ b/tests/xlconfigdata/test-spice-features.cfg
> @@ -8,12 +8,13 @@ acpi = 1
>  apic = 1
>  hap = 0
>  viridian = 0
> +rtc_timeoffset = 0
>  localtime = 0
>  on_poweroff = "destroy"
>  on_reboot = "restart"
>  on_crash = "restart"
>  device_model = "/usr/lib/xen/bin/qemu-dm"
> -vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>  parallel = "none"
>  serial = "none"
>  builder = "hvm"
> diff --git a/tests/xlconfigdata/test-spice-features.xml b/tests/xlconfigdata/test-spice-features.xml
> index 8f3fcf5..10e74e0 100644
> --- a/tests/xlconfigdata/test-spice-features.xml
> +++ b/tests/xlconfigdata/test-spice-features.xml
> @@ -14,7 +14,7 @@
>      <apic/>
>      <pae/>
>    </features>
> -  <clock offset='utc' adjustment='reset'/>
> +  <clock offset='variable' adjustment='0' basis='utc'/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>restart</on_crash>
> diff --git a/tests/xlconfigdata/test-spice.cfg b/tests/xlconfigdata/test-spice.cfg
> index d89f2ba..1a96114 100644
> --- a/tests/xlconfigdata/test-spice.cfg
> +++ b/tests/xlconfigdata/test-spice.cfg
> @@ -8,12 +8,13 @@ acpi = 1
>  apic = 1
>  hap = 0
>  viridian = 0
> +rtc_timeoffset = 0
>  localtime = 0
>  on_poweroff = "destroy"
>  on_reboot = "restart"
>  on_crash = "restart"
>  device_model = "/usr/lib/xen/bin/qemu-dm"
> -vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
>  parallel = "none"
>  serial = "none"
>  builder = "hvm"
> diff --git a/tests/xlconfigdata/test-spice.xml b/tests/xlconfigdata/test-spice.xml
> index e5b43d9..0884d76 100644
> --- a/tests/xlconfigdata/test-spice.xml
> +++ b/tests/xlconfigdata/test-spice.xml
> @@ -14,7 +14,7 @@
>      <apic/>
>      <pae/>
>    </features>
> -  <clock offset='utc' adjustment='reset'/>
> +  <clock offset='variable' adjustment='0' basis='utc'/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>restart</on_crash>
> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c
> index 952b504..08c43fb 100644
> --- a/tests/xlconfigtest.c
> +++ b/tests/xlconfigtest.c
> @@ -193,15 +193,15 @@ mymain(void)
>              ret = -1;                                                   \
>      } while (0)
>  
> -    DO_TEST("new-disk", 3);
> -    DO_TEST("spice", 3);
> -    DO_TEST("spice-features", 3);
> +    DO_TEST("new-disk", 5);
> +    DO_TEST("spice", 5);
> +    DO_TEST("spice-features", 5);
>  
>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
> -    DO_TEST("fullvirt-multiusb", 3);
> +    DO_TEST("fullvirt-multiusb", 5);
>  #endif
>  #ifdef LIBXL_HAVE_BUILDINFO_KERNEL
> -    DO_TEST("fullvirt-direct-kernel-boot", 3);
> +    DO_TEST("fullvirt-direct-kernel-boot", 5);
>  #endif
>  
>      virObjectUnref(caps);