[libvirt] [PATCH] libxl: add support for specifying clock offset and adjustment

[Daniel P. Berrangé]

On Tue, Feb 20, 2018 at 05:28:57PM -0700, Jim Fehlig wrote:
> libxl supports setting the domain real time clock to local time or
> UTC via the localtime field of libxl_domain_build_info. Adjustment
> of the clock is also supported via the rtc_timeoffset field. The
> libvirt libxl driver has never supported these settings, instead
> relying on libxl's default of a UTC real time clock with adjustment
> set to 0.
> 
> There is at least one user that would like the ability to change
> the defaults
> 
> https://www.redhat.com/archives/libvirt-users/2018-February/msg00059.html
> 
> Add support for specifying a local time clock and for specifying an
> adjustment for both local time and UTC clocks. Add a test case to
> verify the XML to libxl_domain_config conversion.
> 
> Local time clock and clock adjustment is already supported by the
> XML <-> xl.cfg converter. What is missing is an explicit test for
> the conversion. There are plenty of existing tests that all use UTC
> with 0 adjustment. Hijack test-fullvirt-tsc-timer to test a local
> time clock with 1 hour adjustment.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>  src/libxl/libxl_conf.c                             | 35 +++++++--
>  .../libxlxml2domconfigdata/variable-clock-hvm.json | 91 ++++++++++++++++++++++
>  .../libxlxml2domconfigdata/variable-clock-hvm.xml  | 36 +++++++++
>  tests/libxlxml2domconfigtest.c                     |  1 +
>  tests/xlconfigdata/test-fullvirt-tsc-timer.cfg     |  4 +-
>  tests/xlconfigdata/test-fullvirt-tsc-timer.xml     |  2 +-
>  6 files changed, 160 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 0c5de344d..39ae709c7 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -274,6 +274,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>                        virCapsPtr caps,
>                        libxl_domain_config *d_config)
>  {
> +    virDomainClockDef clock = def->clock;
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
>      size_t i;
> @@ -293,10 +294,32 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>      for (i = 0; i < virDomainDefGetVcpus(def); i++)
>          libxl_bitmap_set((&b_info->avail_vcpus), i);
>  
> -    for (i = 0; i < def->clock.ntimers; i++) {
> -        switch ((virDomainTimerNameType) def->clock.timers[i]->name) {
> +    switch (clock.offset) {

Can you cast that to virDomainClockOffset to get enum checking

> +    case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE:
> +        if (clock.data.variable.basis == VIR_DOMAIN_CLOCK_BASIS_LOCALTIME)
> +            libxl_defbool_set(&b_info->localtime, true);
> +        b_info->rtc_timeoffset = clock.data.variable.adjustment;
> +        break;
> +
> +    case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
> +        libxl_defbool_set(&b_info->localtime, true);
> +        break;
> +
> +    /* Nothing to do since UTC is the default in libxl */
> +    case VIR_DOMAIN_CLOCK_OFFSET_UTC:
> +        break;
> +

Put case VIR_DOMAIN_CLOCK_OFFSET_LAST: right here

> +    default:
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unsupported clock offset '%s'"),
> +                       virDomainClockOffsetTypeToString(clock.offset));

You shouldn't use the ToString macros in a default: or _LAST: case
because it will return empty string for invalid values. Just print out
the decimal value, and use the word "Unexpected" rather than "unsupported"

> +        return -1;
> +    }

Assuming those simple tweaks are done, then

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|