[libvirt] [PATCH] qemu: Report the offset from host UTC for RTC_CHANGE event
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 (-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 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(+) 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 */ + 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; + virBufferAsprintf(&buf, "base=%d-%02d-%02dT%02d:%02d:%02d", nowbits.tm_year + 1900, nowbits.tm_mon + 1, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d4fd4fb..e6f0b6d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -796,6 +796,18 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); + + /* QEMU's RTC_CHANGE event returns the offset from the specified + * date instead of the host UTC if a specific date is provided + * (-rtc base=$date). We need to convert it to be offset from + * host UTC. + */ + if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) { + time_t now = time(NULL); + + offset += vm->def->clock.data.variable.starttime - now; + } + event = virDomainEventRTCChangeNewFromObj(vm, offset); if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) -- 1.8.1.4
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/06/13 20:59, 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).
Oh, right...
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.
I tried this. virProcessGetStartTime reads the "starttime" from /proc/$pid/stat, which is either jiffies or clock ticks since the system boot. Something like this (sysinfo.uptime is from system boot time too): if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) { if (virProcessGetStartTime(vm->pid, &starttime) < 0) { VIR_WARN("unable to get domain process start time"); } else { struct sysinfo info; ignore_value(sysinfo(&info)); /* Convert domain process's starttime (jiffies) into seconds */ starttime = starttime / sysconf(_SC_CLK_TCK); /* Seconds of date specified with "-rtc=$date" */ rtctime = starttime + vm->def->clock.data.variable.orig_adjustment; /* QEMU's RTC_CHANGE event returns the offset from the specified * date instead of the host UTC if a specific date is provided * to "-rtc" (-rtc=$date). To emit a offset from host UTC, we * need to plus the "offset" from qemu with the "offset" between * the specific date and current host date. */ offset += rtctime - info.uptime; } } Don't mind the "ignore_value(sysinfo(&info))", it's draft code. I changed to store the whole specified date in "starttime" of clock def, because I don't know if there is a portable lib call instead of the "sysinfo", do you have any ideas? Thanks. Osier
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. 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 :|
On 04/06/13 21:13, Daniel P. Berrange wrote:
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.
hm, I don't see libvirt uses "-clock" option. And the offset is from the specified of "-rtc base=$date" with my testing. I'm not familar with qemu code. But it looks the offset is relative to the "-rtc": int qemu_timedate_diff(struct tm *tm) { time_t seconds; if (rtc_date_offset == -1) if (rtc_utc) seconds = mktimegm(tm); else { struct tm tmp = *tm; tmp.tm_isdst = -1; /* use timezone to figure it out */ seconds = mktime(&tmp); } else seconds = mktimegm(tm) + rtc_date_offset; return seconds - time(NULL); } void rtc_change_mon_event(struct tm *tm) { QObject *data; data = qobject_from_jsonf("{ 'offset': %d }", qemu_timedate_diff(tm)); monitor_protocol_event(QEVENT_RTC_CHANGE, data); qobject_decref(data); } Osier
On Tue, Jun 04, 2013 at 09:46:14PM +0800, Osier Yang wrote:
On 04/06/13 21:13, Daniel P. Berrange wrote:
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.
hm, I don't see libvirt uses "-clock" option. And the offset is from the specified of "-rtc base=$date" with my testing.
Sorry, yes, i meant -rtc, not -clock 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 (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang