diff mbox

qemu-ga-win: Fail loudly on bare 'set-time'

Message ID c5be993de2bebdab5c695ef84decc4a35eb8d139.1421838590.git.mprivozn@redhat.com
State New
Headers show

Commit Message

Michal Prívozník Jan. 21, 2015, 11:09 a.m. UTC
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(-)

Comments

Eric Blake Jan. 21, 2015, 11:25 p.m. UTC | #1
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.
Michael Roth Feb. 17, 2015, 1:34 a.m. UTC | #2
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
diff mbox

Patch

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.