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

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(a)suse.com&gt; > --- > 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(a)redhat.com&gt;