Re: [libvirt] [PATCH] qemu: Report the offset from host UTC for RTC_CHANGE event

On Tue, Jun 04, 2013 at 06:59:02AM -0600, Eric Blake wrote: > On 06/04/2013 05:49 AM, Osier Yang wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=964177 >> >> Though both libvirt and QEMU's document say RTC_CHANGE returns >> the offset from the host UTC, qemu actually returns the offset >> from the specified date instead when specific date is privided > s/privided/provided/ > >> (-rtc base=$date). >> >> It's not safe for qemu to fix it in code, it worked like that >> for 3 years, changing it now may break other QEMU use cases. >> What qemu tries to do is to fix the document: >> >> http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg04782.html >> >> And in libvirt side, instead of reply on the qemu, this covert > s/covert/convert/ > >> the offset returned from qemu to the offset from host UTC, by: >> >> /* >> * a: the offset from qemu RTC_CHANGE event >> * b: The specified date (-rtc base=$date) >> * c: the host date when libvirt gets the RTC_CHANGE event >> * offset: What libvirt will report >> */ >> >> offset = a + (b - c); >> >> The specified date (-rtc base=$date) is recorded in clock's def as >> an internal only member (may be useful to exposed outside?). >> --- >> src/conf/domain_conf.h | 3 +++ >> src/qemu/qemu_command.c | 3 +++ >> src/qemu/qemu_process.c | 12 ++++++++++++ >> 3 files changed, 18 insertions(+) > Incomplete. You need to track the start time across libvirtd restarts > (in internal XML) for this to reliably work for an event received after > a restart; you also have to cope with a libvirtd restart not finding the > field in internal XML (because the libvirtd restart was due to upgrading > libvirt in the meantime). > >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >> index 3a71d6c..3947a56 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -1767,6 +1767,9 @@ struct _virDomainClockDef { >> struct { >> long long adjustment; >> int basis; >> + >> + /* Store the start time of guest process, internaly only */ > Spelling; either 'internal' or 'internally' > >> + time_t starttime; >> } variable; >> >> /* Timezone name, when >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index c4a162a..9254525 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -5518,6 +5518,9 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) >> now += def->data.variable.adjustment; >> gmtime_r(&now, &nowbits); >> >> + /* Store the starttime of qemu process */ >> + def->data.variable.starttime = now; > Is there anything we can read out of /proc/nnn for the qemu process that > would give us a more accurate start time? In fact, why not use > virProcessGetStartTime()? And if virProcessGetStartTime is reliable > across libvirtd restarts, then you might not need to store a time_t > starttime in _virDomainClockDef. It isn't the start time of the QEMU process that we care about here. The offset is relative to the timestamp specified via the -clock command line arg. So using QEMU procss startup would be wrong.