[Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Michal Privoznik]

As the description to the guest-set-time states, the command is
there to ease time synchronization after resume. If guest was
suspended for longer period of time, its system time can go off
so badly, that even NTP refuses to set it. That's why the command
was invented: to give users chance to set the time (not
necessarily 100% correct). However, there's is no real need for
us to require users to pass an arbitrary time. Especially if we
can read the correct value from RTC (boiling down to reading
host's time). Hence this commit enables logic:

guest-set-time() == guest-set-time($now_from_rtc)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
diff to v1:
-Fix checkpatch.pl warnings 

 qga/commands-posix.c | 41 +++++++++++++++++++++++++----------------
 qga/commands-win32.c | 34 +++++++++++++++++++++++-----------
 qga/qapi-schema.json |  9 +++++----
 3 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8100bee..7d6665a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -142,7 +142,7 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
-void qmp_guest_set_time(int64_t time_ns, Error **errp)
+void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
 {
     int ret;
     int status;
@@ -150,22 +150,28 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
     Error *local_err = NULL;
     struct timeval tv;
 
-    /* year-2038 will overflow in case time_t is 32bit */
-    if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
-        error_setg(errp, "Time %" PRId64 " is too large", time_ns);
-        return;
+    /* If user has passed a time, validate and set it. */
+    if (has_time) {
+        /* year-2038 will overflow in case time_t is 32bit */
+        if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
+            error_setg(errp, "Time %" PRId64 " is too large", time_ns);
+            return;
+        }
+
+        tv.tv_sec = time_ns / 1000000000;
+        tv.tv_usec = (time_ns % 1000000000) / 1000;
+
+        ret = settimeofday(&tv, NULL);
+        if (ret < 0) {
+            error_setg_errno(errp, errno, "Failed to set time to guest");
+            return;
+        }
     }
 
-    tv.tv_sec = time_ns / 1000000000;
-    tv.tv_usec = (time_ns % 1000000000) / 1000;
-
-    ret = settimeofday(&tv, NULL);
-    if (ret < 0) {
-        error_setg_errno(errp, errno, "Failed to set time to guest");
-        return;
-    }
-
-    /* Set the Hardware Clock to the current System Time. */
+    /* Now, if user has passed a time to set and the system time is set, we
+     * just need to synchronize the hardware clock. However, if no time was
+     * passed, user is requesting the opposite: set the system time from the
+     * hardware clock. */
     pid = fork();
     if (pid == 0) {
         setsid();
@@ -173,7 +179,10 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
         reopen_fd_to_null(1);
         reopen_fd_to_null(2);
 
-        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
+        /* Use '/sbin/hwclock -w' to set RTC from the system time,
+         * or '/sbin/hwclock -s' to set the system time from RTC. */
+        execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
+               NULL, environ);
         _exit(EXIT_FAILURE);
     } else if (pid < 0) {
         error_setg_errno(errp, errno, "failed to create child process");
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a6a0af2..8245f95 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -370,25 +370,37 @@ int64_t qmp_guest_get_time(Error **errp)
     return time_ns;
 }
 
-void qmp_guest_set_time(int64_t time_ns, Error **errp)
+void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
 {
     SYSTEMTIME ts;
     FILETIME tf;
     LONGLONG time;
 
-    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
-        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
-        return;
-    }
+    if (has_time) {
+        /* Okay, user passed a time to set. Validate it. */
+        if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
+            error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
+            return;
+        }
 
-    time = time_ns / 100 + W32_FT_OFFSET;
+        time = time_ns / 100 + W32_FT_OFFSET;
 
-    tf.dwLowDateTime = (DWORD) time;
-    tf.dwHighDateTime = (DWORD) (time >> 32);
+        tf.dwLowDateTime = (DWORD) time;
+        tf.dwHighDateTime = (DWORD) (time >> 32);
 
-    if (!FileTimeToSystemTime(&tf, &ts)) {
-        error_setg(errp, "Failed to convert system time %d", (int)GetLastError());
-        return;
+        if (!FileTimeToSystemTime(&tf, &ts)) {
+            error_setg(errp, "Failed to convert system time %d",
+                       (int)GetLastError());
+            return;
+        }
+    } else {
+        /* Otherwise read the time from RTC which contains the correct value.
+         * Hopefully. */
+        GetSystemTime(&ts);
+        if (ts.wYear < 1601 || ts.wYear > 30827) {
+            error_setg(errp, "Failed to get time");
+            return;
+        }
     }
 
     acquire_privilege(SE_SYSTEMTIME_NAME, errp);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 245f968..80edca1 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -120,17 +120,18 @@
 # This command tries to set guest time to the given value,
 # then sets the Hardware Clock to the current System Time.
 # This will make it easier for a guest to resynchronize
-# without waiting for NTP.
+# without waiting for NTP. If no @time is specified, then
+# the time to set is read from RTC.
 #
-# @time: time of nanoseconds, relative to the Epoch of
-#        1970-01-01 in UTC.
+# @time: #optional time of nanoseconds, relative to the Epoch
+#        of 1970-01-01 in UTC.
 #
 # Returns: Nothing on success.
 #
 # Since: 1.5
 ##
 { 'command': 'guest-set-time',
-  'data': { 'time': 'int' } }
+  'data': { '*time': 'int' } }
 
 ##
 # @GuestAgentCommandInfo:
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Eric Blake]

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

On 01/31/2014 03:29 AM, Michal Privoznik wrote:
> As the description to the guest-set-time states, the command is
> there to ease time synchronization after resume. If guest was
> suspended for longer period of time, its system time can go off
> so badly, that even NTP refuses to set it. That's why the command
> was invented: to give users chance to set the time (not
> necessarily 100% correct). However, there's is no real need for
> us to require users to pass an arbitrary time. Especially if we
> can read the correct value from RTC (boiling down to reading
> host's time). Hence this commit enables logic:
> 
> guest-set-time() == guest-set-time($now_from_rtc)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> diff to v1:
> -Fix checkpatch.pl warnings 
> 
>  qga/commands-posix.c | 41 +++++++++++++++++++++++++----------------
>  qga/commands-win32.c | 34 +++++++++++++++++++++++-----------
>  qga/qapi-schema.json |  9 +++++----
>  3 files changed, 53 insertions(+), 31 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Conversion from mandatory to optional is backward-compatible.  I'm not
sure if it worth some sort of annotation when it was made optional; maybe:

# @time: (#optional since 2.0) time of nanoseconds, relative to the
#        Epoch of 1970-01-01 in UTC.

we don't have much precedent for that sort of change, but documenting it
may make it more obvious to a user that older agents require an argument.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Michal Privoznik]

On 31.01.2014 19:12, Eric Blake wrote:
> On 01/31/2014 03:29 AM, Michal Privoznik wrote:
>> As the description to the guest-set-time states, the command is
>> there to ease time synchronization after resume. If guest was
>> suspended for longer period of time, its system time can go off
>> so badly, that even NTP refuses to set it. That's why the command
>> was invented: to give users chance to set the time (not
>> necessarily 100% correct). However, there's is no real need for
>> us to require users to pass an arbitrary time. Especially if we
>> can read the correct value from RTC (boiling down to reading
>> host's time). Hence this commit enables logic:
>>
>> guest-set-time() == guest-set-time($now_from_rtc)
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> diff to v1:
>> -Fix checkpatch.pl warnings
>>
>>   qga/commands-posix.c | 41 +++++++++++++++++++++++++----------------
>>   qga/commands-win32.c | 34 +++++++++++++++++++++++-----------
>>   qga/qapi-schema.json |  9 +++++----
>>   3 files changed, 53 insertions(+), 31 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

May I kindly ask somebody to merge this? Or is 3ACKs required for the 
guest agent too? Thanks in advance!

Michal

Re: [Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Michael Roth]

Quoting Michal Privoznik (2014-01-31 04:29:51)
> As the description to the guest-set-time states, the command is
> there to ease time synchronization after resume. If guest was
> suspended for longer period of time, its system time can go off
> so badly, that even NTP refuses to set it. That's why the command
> was invented: to give users chance to set the time (not
> necessarily 100% correct). However, there's is no real need for
> us to require users to pass an arbitrary time. Especially if we
> can read the correct value from RTC (boiling down to reading
> host's time). Hence this commit enables logic:
> 
> guest-set-time() == guest-set-time($now_from_rtc)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Thanks, applied to qga tree:                                                                                 

  https://github.com/mdroth/qemu/commits/qga

> ---
> diff to v1:
> -Fix checkpatch.pl warnings 
> 
>  qga/commands-posix.c | 41 +++++++++++++++++++++++++----------------
>  qga/commands-win32.c | 34 +++++++++++++++++++++++-----------
>  qga/qapi-schema.json |  9 +++++----
>  3 files changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8100bee..7d6665a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -142,7 +142,7 @@ int64_t qmp_guest_get_time(Error **errp)
>     return time_ns;
>  }
> 
> -void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>  {
>      int ret;
>      int status;
> @@ -150,22 +150,28 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
>      Error *local_err = NULL;
>      struct timeval tv;
> 
> -    /* year-2038 will overflow in case time_t is 32bit */
> -    if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
> -        error_setg(errp, "Time %" PRId64 " is too large", time_ns);
> -        return;
> +    /* If user has passed a time, validate and set it. */
> +    if (has_time) {
> +        /* year-2038 will overflow in case time_t is 32bit */
> +        if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
> +            error_setg(errp, "Time %" PRId64 " is too large", time_ns);
> +            return;
> +        }
> +
> +        tv.tv_sec = time_ns / 1000000000;
> +        tv.tv_usec = (time_ns % 1000000000) / 1000;
> +
> +        ret = settimeofday(&tv, NULL);
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno, "Failed to set time to guest");
> +            return;
> +        }
>      }
> 
> -    tv.tv_sec = time_ns / 1000000000;
> -    tv.tv_usec = (time_ns % 1000000000) / 1000;
> -
> -    ret = settimeofday(&tv, NULL);
> -    if (ret < 0) {
> -        error_setg_errno(errp, errno, "Failed to set time to guest");
> -        return;
> -    }
> -
> -    /* Set the Hardware Clock to the current System Time. */
> +    /* Now, if user has passed a time to set and the system time is set, we
> +     * just need to synchronize the hardware clock. However, if no time was
> +     * passed, user is requesting the opposite: set the system time from the
> +     * hardware clock. */
>      pid = fork();
>      if (pid == 0) {
>          setsid();
> @@ -173,7 +179,10 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
>          reopen_fd_to_null(1);
>          reopen_fd_to_null(2);
> 
> -        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
> +        /* Use '/sbin/hwclock -w' to set RTC from the system time,
> +         * or '/sbin/hwclock -s' to set the system time from RTC. */
> +        execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
> +               NULL, environ);
>          _exit(EXIT_FAILURE);
>      } else if (pid < 0) {
>          error_setg_errno(errp, errno, "failed to create child process");
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index a6a0af2..8245f95 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -370,25 +370,37 @@ int64_t qmp_guest_get_time(Error **errp)
>      return time_ns;
>  }
> 
> -void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>  {
>      SYSTEMTIME ts;
>      FILETIME tf;
>      LONGLONG time;
> 
> -    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> -        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> -        return;
> -    }
> +    if (has_time) {
> +        /* Okay, user passed a time to set. Validate it. */
> +        if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> +            error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> +            return;
> +        }
> 
> -    time = time_ns / 100 + W32_FT_OFFSET;
> +        time = time_ns / 100 + W32_FT_OFFSET;
> 
> -    tf.dwLowDateTime = (DWORD) time;
> -    tf.dwHighDateTime = (DWORD) (time >> 32);
> +        tf.dwLowDateTime = (DWORD) time;
> +        tf.dwHighDateTime = (DWORD) (time >> 32);
> 
> -    if (!FileTimeToSystemTime(&tf, &ts)) {
> -        error_setg(errp, "Failed to convert system time %d", (int)GetLastError());
> -        return;
> +        if (!FileTimeToSystemTime(&tf, &ts)) {
> +            error_setg(errp, "Failed to convert system time %d",
> +                       (int)GetLastError());
> +            return;
> +        }
> +    } else {
> +        /* Otherwise read the time from RTC which contains the correct value.
> +         * Hopefully. */
> +        GetSystemTime(&ts);
> +        if (ts.wYear < 1601 || ts.wYear > 30827) {
> +            error_setg(errp, "Failed to get time");
> +            return;
> +        }
>      }
> 
>      acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 245f968..80edca1 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -120,17 +120,18 @@
>  # This command tries to set guest time to the given value,
>  # then sets the Hardware Clock to the current System Time.
>  # This will make it easier for a guest to resynchronize
> -# without waiting for NTP.
> +# without waiting for NTP. If no @time is specified, then
> +# the time to set is read from RTC.
>  #
> -# @time: time of nanoseconds, relative to the Epoch of
> -#        1970-01-01 in UTC.
> +# @time: #optional time of nanoseconds, relative to the Epoch
> +#        of 1970-01-01 in UTC.
>  #
>  # Returns: Nothing on success.
>  #
>  # Since: 1.5
>  ##
>  { 'command': 'guest-set-time',
> -  'data': { 'time': 'int' } }
> +  'data': { '*time': 'int' } }
> 
>  ##
>  # @GuestAgentCommandInfo:
> -- 
> 1.8.5.2