[Qemu-devel] [RFC PATCH 0/3] Time resync support by qemu-ga

[Qemu-devel] [RFC PATCH 0/3] Time resync support by qemu-ga

[Lei Li]

This patch series attempts to add time resync support
to qemu-ga by introducing qemu-ga commands guest-get-time
and guest-set-time.

Right now, when a guest is paused or migrated to a file
then loaded from that file, the guest OS has no idea that
there was a big gap in the time. Depending on how long the
gap was, NTP might not be able to resynchronize the guest.
So adding new guest-agent command that is called any time
a guest is resumed  and which tells the guest to update its
own wall clock time based on the information from the host
will make it easier for a guest to resynchronize without
waiting for NTP.

The previous RFC send for discussion and suggestion as link
here:

http://article.gmane.org/gmane.comp.emulators.qemu/186126

The interface for these commands like:

{ 'command': 'guest-get-time', 'returns': 'HostTimeInfo' }

{ 'command': 'guest-set-time',
  'data': { '*seconds': 'int', '*microseconds': 'int',
            '*utc-offset': 'int' } }

TODO:
This is a RFC version with POSIX-specific command implemented.
I just test on Linux guest, will add win32-specific command to
support Windows guest later. Since I want to make sure if this
seems like the way we should be headed.

Your comments and suggestions are very welcome!

Lei Li (3):
  qga: add support to get host time
  qga: add guest-get-time command
  qga: add guest-set-time command

[Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   18 ++++++++++++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a657201..26b0fa0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -91,6 +91,24 @@ exit_err:
     error_set(err, QERR_UNDEFINED_ERROR);
 }
 
+static HostTimeInfo *get_host_time(void)
+{
+    int err;
+    qemu_timeval tq;
+    HostTimeInfo *host_time;
+
+    err = qemu_gettimeofday(&tq);
+    if (err < 0) {
+        return NULL;
+    }
+
+    host_time = g_malloc0(sizeof(HostTimeInfo));
+    host_time->seconds = tq.tv_sec;
+    host_time->microseconds = tq.tv_usec;
+
+    return host_time;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index ed0eb69..7793aff 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -83,6 +83,23 @@
 { 'command': 'guest-ping' }
 
 ##
+# @HostTimeInfo
+#
+# Information about host time.
+#
+# @seconds: "seconds" time from the host.
+#
+# @microseconds: "microseconds" time from the host.
+#
+# @utc-offset: information about utc offset.
+#
+# Since: 1.4
+##
+{ 'type': 'HostTimeInfo',
+  'data': { 'seconds': 'int', 'microseconds': 'int',
+             'utc-offset': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   12 ++++++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26b0fa0..190199d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -109,6 +109,18 @@ static HostTimeInfo *get_host_time(void)
     return host_time;
 }
 
+struct HostTimeInfo *qmp_guest_get_time(Error **errp)
+{
+    HostTimeInfo *host_time = get_host_time();
+
+    if (!host_time) {
+        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
+        return NULL;
+    }
+
+    return host_time;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 7793aff..4a8b93c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -100,6 +100,23 @@
              'utc-offset': 'int' } }
 
 ##
+# @guest-get-time:
+#
+# Get the information about host time in UTC and the
+# UTC offset.
+#
+# This command tries to get the host time which is
+# presumably correct, since need to be able to resynchronize
+# clock to host in guest.
+#
+# Returns: @HostTimeInfo on success.
+#
+# Since 1.4
+##
+{ 'command': 'guest-get-time',
+  'returns': 'HostTimeInfo' }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json |   32 ++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 190199d..7fff49a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp)
     return host_time;
 }
 
+void qmp_guest_set_time(bool has_seconds, int64_t seconds,
+                        bool has_microseconds, int64_t microseconds,
+                        bool has_utc_offset, int64_t utc_offset, Error **errp)
+{
+    int ret;
+    int status;
+    pid_t pid, rpid;
+    struct timeval tv;
+    HostTimeInfo *host_time;
+
+    if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) {
+        host_time = get_host_time();
+        if (!host_time) {
+            error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time");
+            return;
+        }
+        tv.tv_sec = host_time->seconds;
+        tv.tv_usec = host_time->microseconds;
+    } else if (has_seconds && has_microseconds && has_utc_offset) {
+        tv.tv_sec = (time_t) seconds + utc_offset;
+        tv.tv_usec = (time_t) microseconds;
+    } else {
+        error_set(errp, QERR_INVALID_PARAMETER, "parameter missing");
+        return;
+    }
+
+    ret = settimeofday(&tv, NULL);
+    if (ret < 0) {
+        error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));
+        return;
+    }
+
+    /* Set the Hardware Clock to the current System Time. */
+    pid = fork();
+    if (pid == 0) {
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        goto exit_err;
+    }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
+        return;
+    }
+
+exit_err:
+    error_set(errp, QERR_UNDEFINED_ERROR);
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4a8b93c..4649b55 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -117,6 +117,38 @@
   'returns': 'HostTimeInfo' }
 
 ##
+# @guest-set-time:
+#
+# Set guest time. If none arguments were given, will set
+# host time to guest.
+#
+# Right now, when a guest is paused or migrated to a file
+# then loaded from that file, the guest OS has no idea that
+# there was a big gap in the time. Depending on how long
+# the gap was, NTP might not be able to resynchronize the
+# guest.
+#
+# This command tries to set guest time based on the information
+# from host or an absolute value given by management app, and
+# set the Hardware Clock to the current System Time. This
+# will make it easier for a guest to resynchronize without
+# waiting for NTP.
+#
+# @seconds: #optional "seconds" time.
+#
+# @microseconds: #optional "microseconds" time.
+#
+# @utc-offset: #optional utc offset.
+#
+# Returns: Nothing on success.
+#
+# Since: 1.4
+##
+{ 'command': 'guest-set-time',
+  'data': { '*seconds': 'int', '*microseconds': 'int',
+            '*utc-offset': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.
-- 
1.7.7.6

Re: [Qemu-devel] [RFC PATCH 0/3] Time resync support by qemu-ga

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

On 01/06/2013 03:06 AM, Lei Li wrote:
> This patch series attempts to add time resync support
> to qemu-ga by introducing qemu-ga commands guest-get-time
> and guest-set-time.
> 

> 
> The interface for these commands like:
> 
> { 'command': 'guest-get-time', 'returns': 'HostTimeInfo' }
> 
> { 'command': 'guest-set-time',
>   'data': { '*seconds': 'int', '*microseconds': 'int',
>             '*utc-offset': 'int' } }

Why are these fields marked optional?  I guess I'll find out in the
actual patch, but at first glance, I think all three would be mandatory.

> 
> TODO:
> This is a RFC version with POSIX-specific command implemented.
> I just test on Linux guest, will add win32-specific command to
> support Windows guest later. Since I want to make sure if this
> seems like the way we should be headed.

I hope that you just meant that you are lacking a win32 implementation
of the command, but that when it is implemented, it will still use the
same interface.  That is, even though windows itself doesn't use POSIX
time, and has a different Epoch than Jan 1 1970, such differences should
be transparent to the user, who should always pass a POSIX timestamp
through the interface.  What we don't want is two different commands,
where the management app then has to know whether the guest is Unix or
Windows based, to know which of the two commands to send.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]

On 01/06/2013 03:06 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
>  
> +static HostTimeInfo *get_host_time(void)
> +{

> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;

Why usec?  struct timespec with nanoseconds might be a nicer unit, even
if for the initial implementation, you use
qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime
library for full ns resolution.  If nothing else, the lesson that ought
to be learned from the proliferation of time types is that any time you
don't report lots of precision, someone comes along later on having to
add yet another interface adding more precision.


> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
>  
>  ##
> +# @HostTimeInfo
> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.

Document that this is relative to the Epoch of 1970-01-01 (no matter
what the host uses for its internal reference point).

> +#
> +# @microseconds: "microseconds" time from the host.

Again, nanoseconds (struct timespec) might be nicer.

> +#
> +# @utc-offset: information about utc offset.

In what format?  Minutes away from UTC, a 4-digit decimal value, or
something else (that is, is a one-hour offset represented as 60 or 100)?
 Are negative values east or west of UTC?

> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }

Indentation seems inconsistent.

Ah, here you made them mandatory - only your cover letter implied that
they were optional.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On 01/06/2013 03:06 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   12 ++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 

> +++ b/qga/qapi-schema.json
> @@ -100,6 +100,23 @@
>               'utc-offset': 'int' } }
>  
>  ##
> +# @guest-get-time:
> +#
> +# Get the information about host time in UTC and the
> +# UTC offset.

About the host time, or about the guest time?  In other words, doesn't
this command exist for the host to ask the guest what time the _guest_
thinks it is, so that the host can then decide whether to issue a
followup command to tell the guest to adjust its time?

> +#
> +# This command tries to get the host time which is
> +# presumably correct, since need to be able to resynchronize
> +# clock to host in guest.
> +#
> +# Returns: @HostTimeInfo on success.

For that matter, should we name the type in patch 1/3 'TimeInfo',
instead of 'HostTimeInfo', as it is not intrinsically tied to host or
guest, but more a function of who is being queried?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4208 bytes --]

On 01/06/2013 03:07 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json |   32 ++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 190199d..7fff49a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>      return host_time;
>  }
>  
> +void qmp_guest_set_time(bool has_seconds, int64_t seconds,
> +                        bool has_microseconds, int64_t microseconds,
> +                        bool has_utc_offset, int64_t utc_offset, Error **errp)
> +{
> +    int ret;
> +    int status;
> +    pid_t pid, rpid;
> +    struct timeval tv;
> +    HostTimeInfo *host_time;
> +
> +    if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) {

Is it really qemu style to parenthesize this much?

> +        host_time = get_host_time();
> +        if (!host_time) {
> +            error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time");
> +            return;
> +        }
> +        tv.tv_sec = host_time->seconds;
> +        tv.tv_usec = host_time->microseconds;
> +    } else if (has_seconds && has_microseconds && has_utc_offset) {
> +        tv.tv_sec = (time_t) seconds + utc_offset;

You need to worry about overflow on hosts where time_t is 32-bits but
the user passed time using 64-bits (such as past the year 2038).
Likewise, it might be worth bounds-checking utc-offset to be at most 12
hours away from UTC (or is there a better bounds?).

> +        tv.tv_usec = (time_t) microseconds;

Likewise, you should range-validate that microseconds does not overflow
1000000 (or, if you take my suggestion about using nanoseconds, since
struct timespec is a bit more expressive, then bound things by
1000000000, and properly round when converting to lower resolution
interfaces such as settimeofday()).

> +    } else {
> +        error_set(errp, QERR_INVALID_PARAMETER, "parameter missing");

That's a bit harsh.  I'm thinking it might be nicer to support:

all three missing - grab time from the host
at least seconds present - populate any missing subseconds or utc_offset
as 0
seconds missing, but other fields present - error

making this look more like:

if (!has_seconds) {
    if (has_subseconds || has_utc_offset) {
        error_set();
    } else {
        use get_host_time();
    }
} else {
    tv.tv_sec = seconds + (has_utc_offset ? utc_offset : 0);
    ...
}

> +++ b/qga/qapi-schema.json
> @@ -117,6 +117,38 @@
>    'returns': 'HostTimeInfo' }
>  
>  ##
> +# @guest-set-time:
> +#
> +# Set guest time. If none arguments were given, will set

s/none/no/

> +# host time to guest.
> +#
> +# Right now, when a guest is paused or migrated to a file
> +# then loaded from that file, the guest OS has no idea that
> +# there was a big gap in the time. Depending on how long
> +# the gap was, NTP might not be able to resynchronize the
> +# guest.
> +#
> +# This command tries to set guest time based on the information
> +# from host or an absolute value given by management app, and
> +# set the Hardware Clock to the current System Time. This
> +# will make it easier for a guest to resynchronize without
> +# waiting for NTP.
> +#
> +# @seconds: #optional "seconds" time.
> +#
> +# @microseconds: #optional "microseconds" time.
> +#
> +# @utc-offset: #optional utc offset.

If you like my above suggestions, this might be worth documenting that
@microseconds (or @nanoseconds) must not be provided unless @seconds is
present, and so on.

Same questions as in patch 1/3 - you need to document what @seconds is
relative to (presumably the Epoch of 1970-01-01), and what format
utc-offset takes.  Based on this patch, it looks like you are using
utc-offset as the number of seconds difference, so one hour is
represented as 3600.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Luiz Capitulino]

On Sun,  6 Jan 2013 18:06:58 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
>  
> +static HostTimeInfo *get_host_time(void)
> +{

Does this build? Because no one is using this function.

> +    int err;
> +    qemu_timeval tq;
> +    HostTimeInfo *host_time;
> +
> +    err = qemu_gettimeofday(&tq);
> +    if (err < 0) {

I'd recommend taking an Error * argument and setting it with
error_set_errno().

> +        return NULL;
> +    }
> +
> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index ed0eb69..7793aff 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
>  
>  ##
> +# @HostTimeInfo

I'm a bit confused, why do you call it HostTimeInfo if this runs
in the guest?

> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.
> +#
> +# @microseconds: "microseconds" time from the host.
> +#
> +# @utc-offset: information about utc offset.
> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.

Re: [Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[Luiz Capitulino]

On Sun,  6 Jan 2013 18:06:59 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   12 ++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 26b0fa0..190199d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -109,6 +109,18 @@ static HostTimeInfo *get_host_time(void)
>      return host_time;
>  }
>  
> +struct HostTimeInfo *qmp_guest_get_time(Error **errp)
> +{
> +    HostTimeInfo *host_time = get_host_time();

The command is called guest_get_time() and runs in the guest, but it returns
HostTimeInfo. Is this correct?

> +
> +    if (!host_time) {
> +        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
> +        return NULL;
> +    }
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 7793aff..4a8b93c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -100,6 +100,23 @@
>               'utc-offset': 'int' } }
>  
>  ##
> +# @guest-get-time:
> +#
> +# Get the information about host time in UTC and the
> +# UTC offset.
> +#
> +# This command tries to get the host time which is
> +# presumably correct, since need to be able to resynchronize
> +# clock to host in guest.
> +#
> +# Returns: @HostTimeInfo on success.
> +#
> +# Since 1.4
> +##
> +{ 'command': 'guest-get-time',
> +  'returns': 'HostTimeInfo' }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.

Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Luiz Capitulino]

On Sun,  6 Jan 2013 18:07:00 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json |   32 ++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 190199d..7fff49a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>      return host_time;
>  }
>  
> +void qmp_guest_set_time(bool has_seconds, int64_t seconds,
> +                        bool has_microseconds, int64_t microseconds,
> +                        bool has_utc_offset, int64_t utc_offset, Error **errp)
> +{
> +    int ret;
> +    int status;
> +    pid_t pid, rpid;
> +    struct timeval tv;
> +    HostTimeInfo *host_time;
> +
> +    if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) {
> +        host_time = get_host_time();
> +        if (!host_time) {
> +            error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time");

If you change get_host_time() to take an Error * argument, you can drop this.

> +            return;
> +        }
> +        tv.tv_sec = host_time->seconds;
> +        tv.tv_usec = host_time->microseconds;
> +    } else if (has_seconds && has_microseconds && has_utc_offset) {
> +        tv.tv_sec = (time_t) seconds + utc_offset;
> +        tv.tv_usec = (time_t) microseconds;
> +    } else {
> +        error_set(errp, QERR_INVALID_PARAMETER, "parameter missing");

Please, use error_setg() instead.

> +        return;
> +    }
> +
> +    ret = settimeofday(&tv, NULL);
> +    if (ret < 0) {
> +        error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));

Please, use error_setg_errno().

> +        return;
> +    }
> +
> +    /* Set the Hardware Clock to the current System Time. */
> +    pid = fork();
> +    if (pid == 0) {
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);

Honest question: is this really necessary? Can't we do whatever hwclock does?

> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        goto exit_err;
> +    }
> +
> +    do {
> +        rpid = waitpid(pid, &status, 0);
> +    } while (rpid == -1 && errno == EINTR);
> +    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
> +        return;
> +    }
> +
> +exit_err:
> +    error_set(errp, QERR_UNDEFINED_ERROR);
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 4a8b93c..4649b55 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -117,6 +117,38 @@
>    'returns': 'HostTimeInfo' }
>  
>  ##
> +# @guest-set-time:
> +#
> +# Set guest time. If none arguments were given, will set
> +# host time to guest.
> +#
> +# Right now, when a guest is paused or migrated to a file
> +# then loaded from that file, the guest OS has no idea that
> +# there was a big gap in the time. Depending on how long
> +# the gap was, NTP might not be able to resynchronize the
> +# guest.
> +#
> +# This command tries to set guest time based on the information
> +# from host or an absolute value given by management app, and
> +# set the Hardware Clock to the current System Time. This
> +# will make it easier for a guest to resynchronize without
> +# waiting for NTP.
> +#
> +# @seconds: #optional "seconds" time.
> +#
> +# @microseconds: #optional "microseconds" time.
> +#
> +# @utc-offset: #optional utc offset.
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 1.4
> +##
> +{ 'command': 'guest-set-time',
> +  'data': { '*seconds': 'int', '*microseconds': 'int',
> +            '*utc-offset': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[mdroth]

On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
> 
> +static HostTimeInfo *get_host_time(void)
> +{

Should squash this into patch #2, since it doesn't get used
till then. Otherwise we'll break build bisection when compiling
with -Wunused/-Wall && -Werror, which is usually the default.

> +    int err;
> +    qemu_timeval tq;
> +    HostTimeInfo *host_time;
> +
> +    err = qemu_gettimeofday(&tq);
> +    if (err < 0) {
> +        return NULL;
> +    }
> +
> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index ed0eb69..7793aff 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
> 
>  ##
> +# @HostTimeInfo
> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.
> +#
> +# @microseconds: "microseconds" time from the host.
> +#
> +# @utc-offset: information about utc offset.
> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
> -- 
> 1.7.7.6
> 

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Lei Li]

On 01/08/2013 05:52 AM, Eric Blake wrote:
> On 01/06/2013 03:06 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>   
>> +static HostTimeInfo *get_host_time(void)
>> +{
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
> Why usec?  struct timespec with nanoseconds might be a nicer unit, even
> if for the initial implementation, you use
> qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime
> library for full ns resolution.  If nothing else, the lesson that ought
> to be learned from the proliferation of time types is that any time you
> don't report lots of precision, someone comes along later on having to
> add yet another interface adding more precision.

ok, I will have a try to change it to a ns resolution.

>
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>   
>>   ##
>> +# @HostTimeInfo
>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
> Document that this is relative to the Epoch of 1970-01-01 (no matter
> what the host uses for its internal reference point).

Sure.

>
>> +#
>> +# @microseconds: "microseconds" time from the host.
> Again, nanoseconds (struct timespec) might be nicer.
>
>> +#
>> +# @utc-offset: information about utc offset.
> In what format?  Minutes away from UTC, a 4-digit decimal value, or
> something else (that is, is a one-hour offset represented as 60 or 100)?
>   Are negative values east or west of UTC?

For this version, it's a one-hour offset represented as:±[hh].
Negative values are west, andpositive values are east of UTC.

>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
> Indentation seems inconsistent.
>
> Ah, here you made them mandatory - only your cover letter implied that
> they were optional.

Sorry for the inconsistent indentation.
No, this is just the definition of the time structure. The optional arguments
mentioned in the cover letter are for command "guest-set-time". :)



-- 
Lei

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Lei Li]

On 01/09/2013 09:32 PM, Luiz Capitulino wrote:
> On Sun,  6 Jan 2013 18:06:58 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>   
>> +static HostTimeInfo *get_host_time(void)
>> +{
> Does this build? Because no one is using this function.

Yes, this should be squashed into patch #2 as Mike also
pointed out that.

>> +    int err;
>> +    qemu_timeval tq;
>> +    HostTimeInfo *host_time;
>> +
>> +    err = qemu_gettimeofday(&tq);
>> +    if (err < 0) {
> I'd recommend taking an Error * argument and setting it with
> error_set_errno().

ok.

>
>> +        return NULL;
>> +    }
>> +
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index ed0eb69..7793aff 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>   
>>   ##
>> +# @HostTimeInfo
> I'm a bit confused, why do you call it HostTimeInfo if this runs
> in the guest?

I call it HostTimeInfo because it contains the host time information.
But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo'
might be better?

>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
>> +#
>> +# @microseconds: "microseconds" time from the host.
>> +#
>> +# @utc-offset: information about utc offset.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.


-- 
Lei

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Lei Li]

On 01/09/2013 11:36 PM, mdroth wrote:
> On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>
>> +static HostTimeInfo *get_host_time(void)
>> +{
> Should squash this into patch #2, since it doesn't get used
> till then. Otherwise we'll break build bisection when compiling
> with -Wunused/-Wall && -Werror, which is usually the default.

ok, got it.
Thanks!

>> +    int err;
>> +    qemu_timeval tq;
>> +    HostTimeInfo *host_time;
>> +
>> +    err = qemu_gettimeofday(&tq);
>> +    if (err < 0) {
>> +        return NULL;
>> +    }
>> +
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index ed0eb69..7793aff 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>
>>   ##
>> +# @HostTimeInfo
>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
>> +#
>> +# @microseconds: "microseconds" time from the host.
>> +#
>> +# @utc-offset: information about utc offset.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
>> -- 
>> 1.7.7.6
>>


-- 
Lei

Re: [Qemu-devel] [RFC PATCH 0/3] Time resync support by qemu-ga

[Lei Li]

On 01/08/2013 03:01 AM, Eric Blake wrote:
> On 01/06/2013 03:06 AM, Lei Li wrote:
>> This patch series attempts to add time resync support
>> to qemu-ga by introducing qemu-ga commands guest-get-time
>> and guest-set-time.
>>
>> The interface for these commands like:
>>
>> { 'command': 'guest-get-time', 'returns': 'HostTimeInfo' }
>>
>> { 'command': 'guest-set-time',
>>    'data': { '*seconds': 'int', '*microseconds': 'int',
>>              '*utc-offset': 'int' } }
> Why are these fields marked optional?  I guess I'll find out in the
> actual patch, but at first glance, I think all three would be mandatory.
>
>> TODO:
>> This is a RFC version with POSIX-specific command implemented.
>> I just test on Linux guest, will add win32-specific command to
>> support Windows guest later. Since I want to make sure if this
>> seems like the way we should be headed.
> I hope that you just meant that you are lacking a win32 implementation
> of the command, but that when it is implemented, it will still use the
> same interface.  That is, even though windows itself doesn't use POSIX
> time, and has a different Epoch than Jan 1 1970, such differences should
> be transparent to the user, who should always pass a POSIX timestamp
> through the interface.  What we don't want is two different commands,
> where the management app then has to know whether the guest is Unix or
> Windows based, to know which of the two commands to send.

Yes, I mean that there is win32 implementation of this command lacking.
And thanks for your reminder of the differences for the Epoch between
Linux and Windows.



-- 
Lei

Re: [Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[Lei Li]

On 01/08/2013 06:04 AM, Eric Blake wrote:
> On 01/06/2013 03:06 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   12 ++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> +++ b/qga/qapi-schema.json
>> @@ -100,6 +100,23 @@
>>                'utc-offset': 'int' } }
>>   
>>   ##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
>> +# UTC offset.
> About the host time, or about the guest time?  In other words, doesn't
> this command exist for the host to ask the guest what time the _guest_
> thinks it is, so that the host can then decide whether to issue a
> followup command to tell the guest to adjust its time?

No, this command is for getting host time. You might want to take a look at
the RFC and the reply from Mike I sent few days ago for suggestions and
discussions.

http://article.gmane.org/gmane.comp.emulators.qemu/186126

>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since need to be able to resynchronize
>> +# clock to host in guest.
>> +#
>> +# Returns: @HostTimeInfo on success.
> For that matter, should we name the type in patch 1/3 'TimeInfo',
> instead of 'HostTimeInfo', as it is not intrinsically tied to host or
> guest, but more a function of who is being queried?

Yes, it make sense. Luiz feel confused about this 'HostTimeInfo' too,
I think 'TimeInfo' might be a good idea. :)



-- 
Lei

Re: [Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[Lei Li]

On 01/09/2013 09:33 PM, Luiz Capitulino wrote:
> On Sun,  6 Jan 2013 18:06:59 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   12 ++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 26b0fa0..190199d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -109,6 +109,18 @@ static HostTimeInfo *get_host_time(void)
>>       return host_time;
>>   }
>>   
>> +struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>> +{
>> +    HostTimeInfo *host_time = get_host_time();
> The command is called guest_get_time() and runs in the guest, but it returns
> HostTimeInfo. Is this correct?

Okay, looks like this 'HostTimeInfo' brings a lots of confusion. I will change it
to 'TimeInfo' as I replied to another patch.

>> +
>> +    if (!host_time) {
>> +        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
>> +        return NULL;
>> +    }
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index 7793aff..4a8b93c 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -100,6 +100,23 @@
>>                'utc-offset': 'int' } }
>>   
>>   ##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
>> +# UTC offset.
>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since need to be able to resynchronize
>> +# clock to host in guest.
>> +#
>> +# Returns: @HostTimeInfo on success.
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'guest-get-time',
>> +  'returns': 'HostTimeInfo' }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.


-- 
Lei

Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Lei Li]

On 01/08/2013 06:26 AM, Eric Blake wrote:
> On 01/06/2013 03:07 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/qapi-schema.json |   32 ++++++++++++++++++++++++++++
>>   2 files changed, 89 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 190199d..7fff49a 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>>       return host_time;
>>   }
>>   
>> +void qmp_guest_set_time(bool has_seconds, int64_t seconds,
>> +                        bool has_microseconds, int64_t microseconds,
>> +                        bool has_utc_offset, int64_t utc_offset, Error **errp)
>> +{
>> +    int ret;
>> +    int status;
>> +    pid_t pid, rpid;
>> +    struct timeval tv;
>> +    HostTimeInfo *host_time;
>> +
>> +    if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) {
> Is it really qemu style to parenthesize this much?
>
>> +        host_time = get_host_time();
>> +        if (!host_time) {
>> +            error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time");
>> +            return;
>> +        }
>> +        tv.tv_sec = host_time->seconds;
>> +        tv.tv_usec = host_time->microseconds;
>> +    } else if (has_seconds && has_microseconds && has_utc_offset) {
>> +        tv.tv_sec = (time_t) seconds + utc_offset;
> You need to worry about overflow on hosts where time_t is 32-bits but
> the user passed time using 64-bits (such as past the year 2038).
> Likewise, it might be worth bounds-checking utc-offset to be at most 12
> hours away from UTC (or is there a better bounds?).
>
>> +        tv.tv_usec = (time_t) microseconds;
> Likewise, you should range-validate that microseconds does not overflow
> 1000000 (or, if you take my suggestion about using nanoseconds, since
> struct timespec is a bit more expressive, then bound things by
> 1000000000, and properly round when converting to lower resolution
> interfaces such as settimeofday()).
>
>> +    } else {
>> +        error_set(errp, QERR_INVALID_PARAMETER, "parameter missing");
> That's a bit harsh.  I'm thinking it might be nicer to support:
>
> all three missing - grab time from the host
> at least seconds present - populate any missing subseconds or utc_offset
> as 0
> seconds missing, but other fields present - error
>
> making this look more like:
>
> if (!has_seconds) {
>      if (has_subseconds || has_utc_offset) {
>          error_set();
>      } else {
>          use get_host_time();
>      }
> } else {
>      tv.tv_sec = seconds + (has_utc_offset ? utc_offset : 0);
>      ...
> }

Good suggestions!
Yes, I know this is harsh. I will improve it in next version,
as well as the document.

>> +++ b/qga/qapi-schema.json
>> @@ -117,6 +117,38 @@
>>     'returns': 'HostTimeInfo' }
>>   
>>   ##
>> +# @guest-set-time:
>> +#
>> +# Set guest time. If none arguments were given, will set
> s/none/no/
>
>> +# host time to guest.
>> +#
>> +# Right now, when a guest is paused or migrated to a file
>> +# then loaded from that file, the guest OS has no idea that
>> +# there was a big gap in the time. Depending on how long
>> +# the gap was, NTP might not be able to resynchronize the
>> +# guest.
>> +#
>> +# This command tries to set guest time based on the information
>> +# from host or an absolute value given by management app, and
>> +# set the Hardware Clock to the current System Time. This
>> +# will make it easier for a guest to resynchronize without
>> +# waiting for NTP.
>> +#
>> +# @seconds: #optional "seconds" time.
>> +#
>> +# @microseconds: #optional "microseconds" time.
>> +#
>> +# @utc-offset: #optional utc offset.
> If you like my above suggestions, this might be worth documenting that
> @microseconds (or @nanoseconds) must not be provided unless @seconds is
> present, and so on.
>
>
> Same questions as in patch 1/3 - you need to document what @seconds is
> relative to (presumably the Epoch of 1970-01-01), and what format
> utc-offset takes.  Based on this patch, it looks like you are using
> utc-offset as the number of seconds difference, so one hour is
> represented as 3600.

Sure. About the utc-offset format, I have replied to the previous patch.
It would be one-hour offset, and it's my mistake here...:(

>


-- 
Lei

Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Lei Li]

On 01/09/2013 09:40 PM, Luiz Capitulino wrote:
> On Sun,  6 Jan 2013 18:07:00 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/qapi-schema.json |   32 ++++++++++++++++++++++++++++
>>   2 files changed, 89 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 190199d..7fff49a 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>>       return host_time;
>>   }
>>   
>> +void qmp_guest_set_time(bool has_seconds, int64_t seconds,
>> +                        bool has_microseconds, int64_t microseconds,
>> +                        bool has_utc_offset, int64_t utc_offset, Error **errp)
>> +{
>> +    int ret;
>> +    int status;
>> +    pid_t pid, rpid;
>> +    struct timeval tv;
>> +    HostTimeInfo *host_time;
>> +
>> +    if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) {
>> +        host_time = get_host_time();
>> +        if (!host_time) {
>> +            error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time");
> If you change get_host_time() to take an Error * argument, you can drop this.

ok.

>> +            return;
>> +        }
>> +        tv.tv_sec = host_time->seconds;
>> +        tv.tv_usec = host_time->microseconds;
>> +    } else if (has_seconds && has_microseconds && has_utc_offset) {
>> +        tv.tv_sec = (time_t) seconds + utc_offset;
>> +        tv.tv_usec = (time_t) microseconds;
>> +    } else {
>> +        error_set(errp, QERR_INVALID_PARAMETER, "parameter missing");
> Please, use error_setg() instead.

Sure.

>> +        return;
>> +    }
>> +
>> +    ret = settimeofday(&tv, NULL);
>> +    if (ret < 0) {
>> +        error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));
> Please, use error_setg_errno().
>
Yes.

>> +        return;
>> +    }
>> +
>> +    /* Set the Hardware Clock to the current System Time. */
>> +    pid = fork();
>> +    if (pid == 0) {
>> +        setsid();
>> +        reopen_fd_to_null(0);
>> +        reopen_fd_to_null(1);
>> +        reopen_fd_to_null(2);
>> +
>> +        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
> Honest question: is this really necessary? Can't we do whatever hwclock does?
>
I have thought about implementing this ourselves, and I did take a look
at the source code of hwclock. But looks like the implement ofthe synchronization
for hardware clock and system clock is a little complicated, so I am not
sure if it's worth to have a try here.

  

>> +        _exit(EXIT_FAILURE);
>> +    } else if (pid < 0) {
>> +        goto exit_err;
>> +    }
>> +
>> +    do {
>> +        rpid = waitpid(pid, &status, 0);
>> +    } while (rpid == -1 && errno == EINTR);
>> +    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
>> +        return;
>> +    }
>> +
>> +exit_err:
>> +    error_set(errp, QERR_UNDEFINED_ERROR);
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index 4a8b93c..4649b55 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -117,6 +117,38 @@
>>     'returns': 'HostTimeInfo' }
>>   
>>   ##
>> +# @guest-set-time:
>> +#
>> +# Set guest time. If none arguments were given, will set
>> +# host time to guest.
>> +#
>> +# Right now, when a guest is paused or migrated to a file
>> +# then loaded from that file, the guest OS has no idea that
>> +# there was a big gap in the time. Depending on how long
>> +# the gap was, NTP might not be able to resynchronize the
>> +# guest.
>> +#
>> +# This command tries to set guest time based on the information
>> +# from host or an absolute value given by management app, and
>> +# set the Hardware Clock to the current System Time. This
>> +# will make it easier for a guest to resynchronize without
>> +# waiting for NTP.
>> +#
>> +# @seconds: #optional "seconds" time.
>> +#
>> +# @microseconds: #optional "microseconds" time.
>> +#
>> +# @utc-offset: #optional utc offset.
>> +#
>> +# Returns: Nothing on success.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'command': 'guest-set-time',
>> +  'data': { '*seconds': 'int', '*microseconds': 'int',
>> +            '*utc-offset': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
>


-- 
Lei

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Luiz Capitulino]

On Fri, 11 Jan 2013 15:19:35 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> >>   ##
> >> +# @HostTimeInfo
> > I'm a bit confused, why do you call it HostTimeInfo if this runs
> > in the guest?
> 
> I call it HostTimeInfo because it contains the host time information.

qemu-ga runs in the guest, so it's actually the guest time.

> But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo'
> might be better?

Yes, looks better for me.

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

On 01/11/2013 12:18 AM, Lei Li wrote:
> 
> For this version, it's a one-hour offset represented as:±[hh].
> Negative values are west, andpositive values are east of UTC.

Won't work.  There are timezones with a half-hour offset.  You need to
express offset at least in terms of minutes, not just hours.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qga: add guest-get-time command

[mdroth]

On Fri, Jan 11, 2013 at 03:37:26PM +0800, Lei Li wrote:
> On 01/08/2013 06:04 AM, Eric Blake wrote:
> >On 01/06/2013 03:06 AM, Lei Li wrote:
> >>Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >>---
> >>  qga/commands-posix.c |   12 ++++++++++++
> >>  qga/qapi-schema.json |   17 +++++++++++++++++
> >>  2 files changed, 29 insertions(+), 0 deletions(-)
> >>
> >>+++ b/qga/qapi-schema.json
> >>@@ -100,6 +100,23 @@
> >>               'utc-offset': 'int' } }
> >>  ##
> >>+# @guest-get-time:
> >>+#
> >>+# Get the information about host time in UTC and the
> >>+# UTC offset.
> >About the host time, or about the guest time?  In other words, doesn't
> >this command exist for the host to ask the guest what time the _guest_
> >thinks it is, so that the host can then decide whether to issue a
> >followup command to tell the guest to adjust its time?
> 
> No, this command is for getting host time. You might want to take a look at
> the RFC and the reply from Mike I sent few days ago for suggestions and
> discussions.
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/186126

As mentioned elsewhere, what your guest-get-time implementation is actually
returning is the guest time. But that's exactly what we want, since it
provides all the information the host needs to calculate what value it
should pass to guest-set-time (really we just need the utc-offset for this,
but the other values are useful for other use cases. In fact, if you
document guest-*-time to always return values relative to UTC 1970 epoch, we
don't even actually need *that* unless we need to change the guest utc-offset
for some reason, but again, nice to handle other use cases) 

> 
> >>+#
> >>+# This command tries to get the host time which is
> >>+# presumably correct, since need to be able to resynchronize
> >>+# clock to host in guest.
> >>+#
> >>+# Returns: @HostTimeInfo on success.
> >For that matter, should we name the type in patch 1/3 'TimeInfo',
> >instead of 'HostTimeInfo', as it is not intrinsically tied to host or
> >guest, but more a function of who is being queried?
> 
> Yes, it make sense. Luiz feel confused about this 'HostTimeInfo' too,
> I think 'TimeInfo' might be a good idea. :)

Fourth'd :)

> 
> 
> 
> -- 
> Lei
> 

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Lei Li]

On 01/11/2013 11:37 PM, Eric Blake wrote:
> On 01/11/2013 12:18 AM, Lei Li wrote:
>> For this version, it's a one-hour offset represented as:±[hh].
>> Negative values are west, andpositive values are east of UTC.
> Won't work.  There are timezones with a half-hour offset.  You need to
> express offset at least in terms of minutes, not just hours.
>
Yes, I have thought about this. For the timezones with a half-hour offset,
it can just represented as +0.5 for example.



-- 
Lei

Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On 01/13/2013 08:17 PM, Lei Li wrote:
> On 01/11/2013 11:37 PM, Eric Blake wrote:
>> On 01/11/2013 12:18 AM, Lei Li wrote:
>>> For this version, it's a one-hour offset represented as:±[hh].
>>> Negative values are west, andpositive values are east of UTC.
>> Won't work.  There are timezones with a half-hour offset.  You need to
>> express offset at least in terms of minutes, not just hours.
>>
> Yes, I have thought about this. For the timezones with a half-hour offset,
> it can just represented as +0.5 for example.

Floating point parsing is a pain compared to integer parsing.  Remember,
there is no way in POSIX to print a locale-independent floating point
number, which forces programs to jump through quite a few hoops to
guarantee that they are using '.' for the radix character in JSON output
while still honoring the user's locale.

Please, use something based at a minimum on minutes (seconds is also
possible), not hours,

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]