[libvirt] [PATCH 0/2] Allow localtime clock basis

With a nice test enhancement ... Michal Privoznik (2): qemuBuildClockArgStr: Allow localtime clock basis qemuxml2argvtest: Test localtime clock basis src/qemu/qemu_command.c | 18 +++++++---- tests/Makefile.am | 7 ++++ ...muxml2argv-clock-localtime-basis-localtime.args | 5 +++ ...emuxml2argv-clock-localtime-basis-localtime.xml | 28 ++++++++++++++++ tests/qemuxml2argvmock.c | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +- 6 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.xml create mode 100644 tests/qemuxml2argvmock.c -- 1.8.5.2

https://bugzilla.redhat.com/show_bug.cgi?id=1046192 Commit b8bf79a, which adds clock='variable', forgets to check localtime basis in qemuBuildClockArgStr(). So that localtime basis could not be used. Reported-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5b94de1..6cc32f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6491,14 +6491,18 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) time_t now = time(NULL); struct tm nowbits; - if (def->data.variable.basis != VIR_DOMAIN_CLOCK_BASIS_UTC) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported clock basis '%s'"), - virDomainClockBasisTypeToString(def->data.variable.basis)); - goto error; + switch ((enum virDomainClockBasis) def->data.variable.basis) { + case VIR_DOMAIN_CLOCK_BASIS_UTC: + now += def->data.variable.adjustment; + gmtime_r(&now, &nowbits); + break; + case VIR_DOMAIN_CLOCK_BASIS_LOCALTIME: + now += def->data.variable.adjustment; + localtime_r(&now, &nowbits); + break; + case VIR_DOMAIN_CLOCK_BASIS_LAST: + break; } - now += def->data.variable.adjustment; - gmtime_r(&now, &nowbits); /* Store the guest's basedate */ def->data.variable.basedate = now; -- 1.8.5.2

On 02/05/2014 07:32 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1046192
Commit b8bf79a, which adds clock='variable', forgets to check localtime basis in qemuBuildClockArgStr(). So that localtime basis could not be used.
Reported-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

When trying to introduce a test for previous patch, I've noticed that the command line is constructed using current time. This won't work in our test suite (unless you guys wants to set a specific time prior to each test run :) ). Therefore we need to mock calls to time(2) to return the same value every time it's called. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 7 ++++ ...muxml2argv-clock-localtime-basis-localtime.args | 5 +++ ...emuxml2argv-clock-localtime-basis-localtime.xml | 28 ++++++++++++++++ tests/qemuxml2argvmock.c | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +- 5 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.xml create mode 100644 tests/qemuxml2argvmock.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 30cdd1d..eb96f38 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -325,6 +325,7 @@ test_libraries = libshunload.la \ virnetserverclientmock.la \ vircgroupmock.la \ virpcimock.la \ + qemuxml2argvmock.la \ $(NULL) if WITH_QEMU test_libraries += libqemumonitortestutils.la @@ -441,6 +442,12 @@ qemuxml2argvtest_SOURCES = \ testutils.c testutils.h qemuxml2argvtest_LDADD = $(qemu_LDADDS) $(LIBXML_LIBS) +qemuxml2argvmock_la_SOURCES = \ + qemuxml2argvmock.c +qemuxml2argvmock_la_CFLAGS = $(AM_CFLAGS) +qemuxml2argvmock_la_LDFLAGS = -module -avoid-version \ + -rpath /evil/libtool/hack/to/force/shared/lib/creation + qemuxml2xmltest_SOURCES = \ qemuxml2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args b/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args new file mode 100644 index 0000000..24fe89c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -rtc base=2009-02-14T01:31:30 \ +-no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.xml b/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.xml new file mode 100644 index 0000000..91d57b9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1c15a1f6-f4f0-4d3c-9002-667ddb458736</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='variable' adjustment='3600' basis='localtime'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c new file mode 100644 index 0000000..ed9fb13 --- /dev/null +++ b/tests/qemuxml2argvmock.c @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#ifdef __linux__ +# include "internal.h" +# include <time.h> + +time_t time(time_t *t) +{ + const time_t ret = 1234567890; + if (t) + *t = ret; + return ret; +} + +#else +/* Nothing to override on non-__linux__ platforms */ +#endif diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a25264e..6eba74d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -618,6 +618,7 @@ mymain(void) DO_TEST("bios", QEMU_CAPS_DEVICE, QEMU_CAPS_SGA); DO_TEST("clock-utc", NONE); DO_TEST("clock-localtime", NONE); + DO_TEST("clock-localtime-basis-localtime", QEMU_CAPS_RTC); /* * Can't be enabled since the absolute timestamp changes every time DO_TEST("clock-variable", QEMU_CAPS_RTC); @@ -1335,7 +1336,7 @@ mymain(void) return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIRT_TEST_MAIN(mymain) +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemuxml2argvmock.so") #else -- 1.8.5.2

On 02/05/2014 07:32 AM, Michal Privoznik wrote:
When trying to introduce a test for previous patch, I've noticed that the command line is constructed using current time. This won't work in our test suite (unless you guys wants to set a specific time prior to each test run :) ). Therefore we need to mock calls to time(2) to return the same value every time it's called.
Slick. But as we only support mocking calls on Linux, you have converted a test from generic platform to Linux-only. Then again, qemu tests only run where we support qemu, which is currently Linux only, so I'm not sure it will matter. I guess we'll find out if anyone complains that it broke 'make check' on their platform. The only other alternative I can think of is to use regex replacement - in several tests, we have means of munging actual output to recognize specific patterns and replacing them with fixed contents. A timestamp is an easy fixed pattern, and it might be more portable to avoid mocking time() and instead just munge all timestamps of actual output into the expected timestamp. But munging is not quite as precise as your approach of a known fixed point in time - especially if we end up testing multiple expected outputs that have different resulting offsets in relation to the same starting fixed point in time. So I can live with your patch, rather than trying to do regex replacement.
--- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime-basis-localtime.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -rtc base=2009-02-14T01:31:30 \
A quick grep finds: tests/qemuxml2argvdata/qemuxml2argv-clock-variable.args:base=2010-2-2T18:22:10 -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net none \ so how was that test working, and why is it unchanged by your mock setting the time to 2009?
+++ b/tests/qemuxml2argvtest.c @@ -618,6 +618,7 @@ mymain(void) DO_TEST("bios", QEMU_CAPS_DEVICE, QEMU_CAPS_SGA); DO_TEST("clock-utc", NONE); DO_TEST("clock-localtime", NONE); + DO_TEST("clock-localtime-basis-localtime", QEMU_CAPS_RTC); /* * Can't be enabled since the absolute timestamp changes every time DO_TEST("clock-variable", QEMU_CAPS_RTC);
Oh, it _wasn't_ working, but now can be MADE to work. Please uncomment this test, and fix the fallout, at which point, you have: ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Eric Blake wrote:
On 02/05/2014 07:32 AM, Michal Privoznik wrote:
When trying to introduce a test for previous patch, I've noticed that the command line is constructed using current time. This won't work in our test suite (unless you guys wants to set a specific time prior to each test run :) ). Therefore we need to mock calls to time(2) to return the same value every time it's called.
Slick. But as we only support mocking calls on Linux, you have converted a test from generic platform to Linux-only. Then again, qemu tests only run where we support qemu, which is currently Linux only, so I'm not sure it will matter. I guess we'll find out if anyone complains that it broke 'make check' on their platform.
I'm not sure if I have a good understanding of what 'supported' means here, but at least I can say that qemu driver is 'functional' on FreeBSD with some known limitations (i.e. missing firewalling part in bridge driver, etc). Roman Bogorodskiy

On Wed, Feb 05, 2014 at 03:26:23PM -0700, Eric Blake wrote:
On 02/05/2014 07:32 AM, Michal Privoznik wrote:
When trying to introduce a test for previous patch, I've noticed that the command line is constructed using current time. This won't work in our test suite (unless you guys wants to set a specific time prior to each test run :) ). Therefore we need to mock calls to time(2) to return the same value every time it's called.
Slick. But as we only support mocking calls on Linux, you have converted a test from generic platform to Linux-only. Then again, qemu tests only run where we support qemu, which is currently Linux only, so I'm not sure it will matter. I guess we'll find out if anyone complains that it broke 'make check' on their platform.
FYI as a general rule the LD_PRELOAD approach should be capable of working on any ELF platform, which is basically everything we care about except Windows. The cwrap project from samba which uses the same LD_PRELOAD trick claims to have validated such portability. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange
-
Eric Blake
-
Michal Privoznik
-
Roman Bogorodskiy