[Qemu-devel] [PATCH] qemu-ga-win: Fail loudly on bare 'set-time'

[Qemu-devel] [PATCH] qemu-ga-win: Fail loudly on bare 'set-time'

[Michal Privoznik]

The command is not implemented correctly yet. The documentation allows
to not pass any value to set, in which case the time is re-read from
RTC. However, reading CMOS on Windows is not trivial to implement. So
instead of pretending we've set the correct time, fail explicitly.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 qga/commands-win32.c | 44 ++++++++++++++++++++++----------------------
 qga/qapi-schema.json |  5 ++++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3bcbeae..4fe45f0 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -395,31 +395,31 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     FILETIME tf;
     LONGLONG time;
 
-    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;
-        }
+    if (!has_time) {
+        /* Unfortunately, Windows libraries don't provide an easy way to access
+         * RTC yet:
+         *
+         * https://msdn.microsoft.com/en-us/library/aa908981.aspx
+         */
+        error_setg(errp, "Time argument is required on this platform");
+        return;
+    }
 
-        time = time_ns / 100 + W32_FT_OFFSET;
+    /* Validate time passed by user. */
+    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
+        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
+        return;
+    }
 
-        tf.dwLowDateTime = (DWORD) time;
-        tf.dwHighDateTime = (DWORD) (time >> 32);
+    time = time_ns / 100 + W32_FT_OFFSET;
 
-        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;
-        }
+    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;
     }
 
     acquire_privilege(SE_SYSTEMTIME_NAME, &local_err);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 376e79f..ce73dd3 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -121,7 +121,10 @@
 # given value, then sets the Hardware Clock (RTC) to the
 # current System Time. This will make it easier for a guest
 # to resynchronize without waiting for NTP. If no @time is
-# specified, then the time to set is read from RTC.
+# specified, then the time to set is read from RTC. However,
+# this may not be supported on all platforms (i.e. Windows).
+# If that's the case users are advised to always pass a
+# value.
 #
 # @time: #optional time of nanoseconds, relative to the Epoch
 #        of 1970-01-01 in UTC.
-- 
2.0.5

Re: [Qemu-devel] [PATCH] qemu-ga-win: Fail loudly on bare 'set-time'

[Eric Blake]

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

On 01/21/2015 04:09 AM, Michal Privoznik wrote:
> The command is not implemented correctly yet. The documentation allows
> to not pass any value to set, in which case the time is re-read from
> RTC. However, reading CMOS on Windows is not trivial to implement. So
> instead of pretending we've set the correct time, fail explicitly.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  qga/commands-win32.c | 44 ++++++++++++++++++++++----------------------
>  qga/qapi-schema.json |  5 ++++-
>  2 files changed, 26 insertions(+), 23 deletions(-)

Harder to read because of indentation change, but fairly simple. I'm
okay with the code change, but I'll leave it to someone more familiar
with Windows on whether this is the right technical decision.

-- 
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] qemu-ga-win: Fail loudly on bare 'set-time'

[Michael Roth]

Quoting Michal Privoznik (2015-01-21 05:09:50)
> The command is not implemented correctly yet. The documentation allows
> to not pass any value to set, in which case the time is re-read from
> RTC. However, reading CMOS on Windows is not trivial to implement. So
> instead of pretending we've set the correct time, fail explicitly.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Thanks, applied to QGA tree:

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

> ---
>  qga/commands-win32.c | 44 ++++++++++++++++++++++----------------------
>  qga/qapi-schema.json |  5 ++++-
>  2 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3bcbeae..4fe45f0 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -395,31 +395,31 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>      FILETIME tf;
>      LONGLONG time;
> 
> -    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;
> -        }
> +    if (!has_time) {
> +        /* Unfortunately, Windows libraries don't provide an easy way to access
> +         * RTC yet:
> +         *
> +         * https://msdn.microsoft.com/en-us/library/aa908981.aspx
> +         */
> +        error_setg(errp, "Time argument is required on this platform");
> +        return;
> +    }
> 
> -        time = time_ns / 100 + W32_FT_OFFSET;
> +    /* Validate time passed by user. */
> +    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> +        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> +        return;
> +    }
> 
> -        tf.dwLowDateTime = (DWORD) time;
> -        tf.dwHighDateTime = (DWORD) (time >> 32);
> +    time = time_ns / 100 + W32_FT_OFFSET;
> 
> -        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;
> -        }
> +    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;
>      }
> 
>      acquire_privilege(SE_SYSTEMTIME_NAME, &local_err);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 376e79f..ce73dd3 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -121,7 +121,10 @@
>  # given value, then sets the Hardware Clock (RTC) to the
>  # current System Time. This will make it easier for a guest
>  # to resynchronize without waiting for NTP. If no @time is
> -# specified, then the time to set is read from RTC.
> +# specified, then the time to set is read from RTC. However,
> +# this may not be supported on all platforms (i.e. Windows).
> +# If that's the case users are advised to always pass a
> +# value.
>  #
>  # @time: #optional time of nanoseconds, relative to the Epoch
>  #        of 1970-01-01 in UTC.
> -- 
> 2.0.5