[Qemu-devel] [PATCH 0/2 v5] Add Windows support for time resync by qemu-ga

[Qemu-devel] [PATCH 0/2 v5] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was tested on Windows XP SP3 and Windows 7.
Please comment!

Thanks.

Changes since v4:
  - Error handel improvement from Michael.
  - Do the math explicitly for the time convert of FILETIME
    suggested by Michael.

Changes since v3:
  - Reorder the acquire_privilege to avoid a possible
    leak of privileges suggested by Eric.

Changes since v2:
  - Overflow check improvement for time_ns from Eric.

Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 1/2] qga: add windows implementation for guest-get-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..e24fb4a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -22,6 +22,12 @@
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
 #endif
 
+/* multiple of 100 nanoseconds elapsed between windows baseline
+   (1/1/1601) and Unix Epoch (1/1/1970), accounting for leap years */
+#define W32_FT_OFFSET (10000000ULL * 60 * 60 * 24 * \
+                       (365 * (1970 - 1601) +       \
+                        (1970 - 1601) / 4 - 3))
+
 static void acquire_privilege(const char *name, Error **err)
 {
     HANDLE token;
@@ -108,6 +114,33 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     }
 }
 
+int64_t qmp_guest_get_time(Error **errp)
+{
+    SYSTEMTIME *ts = g_malloc0(sizeof(SYSTEMTIME));
+    int64_t time_ns;
+    FILETIME tf;
+
+    GetSystemTime(ts);
+    if (!ts) {
+        error_setg(errp, "Failed to get time");
+        goto out;
+    }
+
+    if (!SystemTimeToFileTime(ts, &tf)) {
+        error_setg_errno(errp, errno, "Failed to convert system time");
+        goto out;
+    }
+
+    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
+                - W32_FT_OFFSET) * 100;
+
+    return time_ns;
+
+out:
+    g_free(ts);
+    return -1;
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/2] qga: add windows implementation for guest-get-time

[mdroth]

On Thu, Mar 14, 2013 at 03:07:50PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 7e8ecb3..e24fb4a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -22,6 +22,12 @@
>  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>  #endif
> 
> +/* multiple of 100 nanoseconds elapsed between windows baseline
> +   (1/1/1601) and Unix Epoch (1/1/1970), accounting for leap years */
> +#define W32_FT_OFFSET (10000000ULL * 60 * 60 * 24 * \
> +                       (365 * (1970 - 1601) +       \
> +                        (1970 - 1601) / 4 - 3))
> +
>  static void acquire_privilege(const char *name, Error **err)
>  {
>      HANDLE token;
> @@ -108,6 +114,33 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>      }
>  }
> 
> +int64_t qmp_guest_get_time(Error **errp)
> +{
> +    SYSTEMTIME *ts = g_malloc0(sizeof(SYSTEMTIME));

I still don't understand why we do just do:

SYSTEM ts = {0};

> +    int64_t time_ns;
> +    FILETIME tf;
> +
> +    GetSystemTime(ts);

followed by:

GetSystemTime(&ts);

(and same for SystemTimeToFileTime() below)

This would avoid the need to add common cleanup code for all the
return paths.

> +    if (!ts) {

this is gonna always be false since we initialize it at the start of
this function.

also, GetSystemTime() does seem to provide any error indication
whatsoever, which is strange. But it also doesn't seem to have any
guarantee this it will always succeed...

I think the best we could do is probably just some kind of sanity
check, like making sure ts.wYear != 0, or maybe that
1601 <= ts.wYear <= 30827

> +        error_setg(errp, "Failed to get time");
> +        goto out;
> +    }
> +
> +    if (!SystemTimeToFileTime(ts, &tf)) {
> +        error_setg_errno(errp, errno, "Failed to convert system time");
> +        goto out;
> +    }
> +
> +    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> +                - W32_FT_OFFSET) * 100;
> +
> +    return time_ns;
> +
> +out:
> +    g_free(ts);
> +    return -1;
> +}
> +
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>  {
>      error_set(err, QERR_UNSUPPORTED);
> -- 
> 1.7.11.7
> 

Re: [Qemu-devel] [PATCH 1/2] qga: add windows implementation for guest-get-time

[Lei Li]

On 03/14/2013 08:32 PM, mdroth wrote:
> On Thu, Mar 14, 2013 at 03:07:50PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-win32.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 7e8ecb3..e24fb4a 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -22,6 +22,12 @@
>>   #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>>   #endif
>>
>> +/* multiple of 100 nanoseconds elapsed between windows baseline
>> +   (1/1/1601) and Unix Epoch (1/1/1970), accounting for leap years */
>> +#define W32_FT_OFFSET (10000000ULL * 60 * 60 * 24 * \
>> +                       (365 * (1970 - 1601) +       \
>> +                        (1970 - 1601) / 4 - 3))
>> +
>>   static void acquire_privilege(const char *name, Error **err)
>>   {
>>       HANDLE token;
>> @@ -108,6 +114,33 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>>       }
>>   }
>>
>> +int64_t qmp_guest_get_time(Error **errp)
>> +{
>> +    SYSTEMTIME *ts = g_malloc0(sizeof(SYSTEMTIME));
> I still don't understand why we do just do:
>
> SYSTEM ts = {0};
>
>> +    int64_t time_ns;
>> +    FILETIME tf;
>> +
>> +    GetSystemTime(ts);
> followed by:
>
> GetSystemTime(&ts);
>
> (and same for SystemTimeToFileTime() below)
>
> This would avoid the need to add common cleanup code for all the
> return paths.
>
>> +    if (!ts) {
> this is gonna always be false since we initialize it at the start of
> this function.
>
> also, GetSystemTime() does seem to provide any error indication
> whatsoever, which is strange. But it also doesn't seem to have any
> guarantee this it will always succeed...
>
> I think the best we could do is probably just some kind of sanity
> check, like making sure ts.wYear != 0, or maybe that
> 1601 <= ts.wYear <= 30827

I was trying to check it by ts.wYear != 0 this afternoon...
OK, I will send update later as you suggested.

Thanks!

>
>> +        error_setg(errp, "Failed to get time");
>> +        goto out;
>> +    }
>> +
>> +    if (!SystemTimeToFileTime(ts, &tf)) {
>> +        error_setg_errno(errp, errno, "Failed to convert system time");
>> +        goto out;
>> +    }
>> +
>> +    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
>> +                - W32_FT_OFFSET) * 100;
>> +
>> +    return time_ns;
>> +
>> +out:
>> +    g_free(ts);
>> +    return -1;
>> +}
>> +
>>   int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>>   {
>>       error_set(err, QERR_UNSUPPORTED);
>> -- 
>> 1.7.11.7
>>


-- 
Lei

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index e24fb4a..8064c3a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -141,6 +141,39 @@ out:
     return -1;
 }
 
+void qmp_guest_set_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;
+    }
+
+    time = time_ns / 100 + W32_FT_OFFSET;
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        error_setg(errp, "Failed to acquire privilege");
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        error_setg(errp, "Failed to set time to guest: %d", (int)GetLastError());
+        return;
+    }
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[mdroth]

On Thu, Mar 14, 2013 at 03:07:51PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index e24fb4a..8064c3a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -141,6 +141,39 @@ out:
>      return -1;
>  }
> 
> +void qmp_guest_set_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;
> +    }
> +
> +    time = time_ns / 100 + W32_FT_OFFSET;
> +
> +    tf.dwLowDateTime = (DWORD) time;
> +    tf.dwHighDateTime = (DWORD) (time >> 32);
> +
> +    if (!FileTimeToSystemTime(&tf, &ts)) {
> +        error_setg(errp, "Failed to convert system time");
> +        return;
> +    }
> +
> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> +    if (error_is_set(errp)) {
> +        error_setg(errp, "Failed to acquire privilege");

Sorry, missed this one on the last review:

Overriding a previously set error will cause an assert. We should just
return and pass back the error from acquire_privilege.

> +        return;
> +    }
> +
> +    if (!SetSystemTime(&ts)) {
> +        error_setg(errp, "Failed to set time to guest: %d", (int)GetLastError());
> +        return;
> +    }
> +}
> +
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>  {
>      error_set(err, QERR_UNSUPPORTED);
> -- 
> 1.7.11.7
> 

[Qemu-devel] [PATCH 0/2 v7] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was tested on Windows XP SP3 and Windows 7.
Please comment!

Thanks.

Changes since v6:
  - Fix other error handel that I missed.

Changes since v5:
  - Fix the error check for GetSystemTime() from Michael.
  - Other fixups from Michael.

Changes since v4:
  - Error handel improvement from Michael.
  - Do the math explicitly for the time convert of FILETIME
    suggested by Michael.

Changes since v3:
  - Reorder the acquire_privilege to avoid a possible
    leak of privileges suggested by Eric.

Changes since v2:
  - Overflow check improvement for time_ns from Eric.

Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d98e3ee..24e4ad0 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -309,7 +309,34 @@ int64_t qmp_guest_get_time(Error **errp)
 
 void qmp_guest_set_time(int64_t time_ns, Error **errp)
 {
-    error_set(errp, QERR_UNSUPPORTED);
+    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;
+    }
+
+    time = time_ns / 100 + W32_FT_OFFSET;
+
+    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, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        error_setg(errp, "Failed to set time to guest: %d", (int)GetLastError());
+        return;
+    }
 }
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
-- 
1.7.11.7

[Qemu-devel] [PATCH 0/2 v6] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was tested on Windows XP SP3 and Windows 7.
Please comment!

Thanks.

Changes since v5:
  - Fix the error check for GetSystemTime() from Michael.
  - Other fixups from Michael.

Changes since v4:
  - Error handel improvement from Michael.
  - Do the math explicitly for the time convert of FILETIME
    suggested by Michael.

Changes since v3:
  - Reorder the acquire_privilege to avoid a possible
    leak of privileges suggested by Eric.

Changes since v2:
  - Overflow check improvement for time_ns from Eric.

Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b395bd5..cc8d890 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -137,6 +137,38 @@ int64_t qmp_guest_get_time(Error **errp)
     return time_ns;
 }
 
+void qmp_guest_set_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;
+    }
+
+    time = time_ns / 100 + W32_FT_OFFSET;
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        error_setg(errp, "Failed to set time to guest: %d", (int)GetLastError());
+        return;
+    }
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.11.7

[Qemu-devel] [PATCH 0/2 v4] Add Windows support for time resync by qemu-ga

[lilei]

From: Lei Li <lilei@linux.vnet.ibm.com>

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was tested on Windows XP SP3 and Windows 7.
Please comment!

Thanks.

Changes since v3:
  - Reorder the acquire_privilege to avoid a possible
    leak of privileges suggested by Eric.

Changes since v2:
  - Overflow check improvement for time_ns from Eric.

Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[lilei]

From: Lei Li <lilei@linux.vnet.ibm.com>

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0a2bb34..a0c8d43 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -140,6 +140,40 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
+void qmp_guest_set_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;
+    }
+
+    time = time_ns / 100 + W32_FT_OFFSET;
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        error_setg(errp, "Failed to acquire privilege");
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        slog("guest-set-time failed: %d", GetLastError());
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        return;
+    }
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[mdroth]

On Wed, Mar 13, 2013 at 06:10:31PM +0800, lilei@linux.vnet.ibm.com wrote:
> From: Lei Li <lilei@linux.vnet.ibm.com>
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 0a2bb34..a0c8d43 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -140,6 +140,40 @@ int64_t qmp_guest_get_time(Error **errp)
>     return time_ns;
>  }
> 
> +void qmp_guest_set_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;
> +    }
> +
> +    time = time_ns / 100 + W32_FT_OFFSET;
> +
> +    tf.dwLowDateTime = (DWORD) time;
> +    tf.dwHighDateTime = (DWORD) (time >> 32);
> +
> +    if (!FileTimeToSystemTime(&tf, &ts)) {
> +        error_setg(errp, "Failed to convert system time");
> +        return;
> +    }
> +
> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> +    if (error_is_set(errp)) {
> +        error_setg(errp, "Failed to acquire privilege");
> +        return;
> +    }
> +
> +    if (!SetSystemTime(&ts)) {
> +        slog("guest-set-time failed: %d", GetLastError());
> +        error_setg_errno(errp, errno, "Failed to set time to guest");

I think we want to pass back GetLastError(), SetSystemTime doesn't set errno
(and if it did something in the slog() callchain probably would've
clobbered it)

Looks good otherwise.

> +        return;
> +    }
> +}
> +
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>  {
>      error_set(err, QERR_UNSUPPORTED);
> -- 
> 1.7.7.6
> 

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

On 03/14/2013 04:25 AM, mdroth wrote:
> On Wed, Mar 13, 2013 at 06:10:31PM +0800, lilei@linux.vnet.ibm.com wrote:
>> From: Lei Li <lilei@linux.vnet.ibm.com>
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-win32.c |   34 ++++++++++++++++++++++++++++++++++
>>   1 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 0a2bb34..a0c8d43 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -140,6 +140,40 @@ int64_t qmp_guest_get_time(Error **errp)
>>      return time_ns;
>>   }
>>
>> +void qmp_guest_set_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;
>> +    }
>> +
>> +    time = time_ns / 100 + W32_FT_OFFSET;
>> +
>> +    tf.dwLowDateTime = (DWORD) time;
>> +    tf.dwHighDateTime = (DWORD) (time >> 32);
>> +
>> +    if (!FileTimeToSystemTime(&tf, &ts)) {
>> +        error_setg(errp, "Failed to convert system time");
>> +        return;
>> +    }
>> +
>> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
>> +    if (error_is_set(errp)) {
>> +        error_setg(errp, "Failed to acquire privilege");
>> +        return;
>> +    }
>> +
>> +    if (!SetSystemTime(&ts)) {
>> +        slog("guest-set-time failed: %d", GetLastError());
>> +        error_setg_errno(errp, errno, "Failed to set time to guest");
> I think we want to pass back GetLastError(), SetSystemTime doesn't set errno
> (and if it did something in the slog() callchain probably would've
> clobbered it)

OK.

> Looks good otherwise.
>
>> +        return;
>> +    }
>> +}
>> +
>>   int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>>   {
>>       error_set(err, QERR_UNSUPPORTED);
>> -- 
>> 1.7.7.6
>>


-- 
Lei

[Qemu-devel] [PATCH 0/2 v3] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was tested on Windows XP SP3 and Windows 7.
Please comment!

Thanks.

Changes since v2:
  - Overflow check improvement for time_ns from Eric.

Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0a2bb34..e000324 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -140,6 +140,40 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    SYSTEMTIME ts;
+    FILETIME tf;
+    LONGLONG time;
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        error_setg(errp, "Failed to acquire privilege");
+        return;
+    }
+
+    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;
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        slog("guest-set-time failed: %d", GetLastError());
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        return;
+    }
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Eric Blake]

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

On 03/12/2013 03:08 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 

> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +{
> +    SYSTEMTIME ts;
> +    FILETIME tf;
> +    LONGLONG time;
> +
> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> +    if (error_is_set(errp)) {
> +        error_setg(errp, "Failed to acquire privilege");
> +        return;
> +    }

Earlier, you told me that acquire_privilege is auto-dropped after a
successful SetSystemTime.  But here, you acquire the privilege...

> +
> +    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> +        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> +        return;

...then return early without ever relinquishing it.

> +    }
> +
> +    time = time_ns / 100 + W32_FT_OFFSET;
> +
> +    tf.dwLowDateTime = (DWORD) time;
> +    tf.dwHighDateTime = (DWORD) (time >> 32);
> +
> +    if (!FileTimeToSystemTime(&tf, &ts)) {
> +        error_setg(errp, "Failed to convert system time");
> +        return;
> +    }

I would reorder the acquire_privilege to here, to give us the best
possible chance of avoiding a leak of privileges when the user passes
bogus data.

> +
> +    if (!SetSystemTime(&ts)) {
> +        slog("guest-set-time failed: %d", GetLastError());
> +        error_setg_errno(errp, errno, "Failed to set time to guest");
> +        return;
> +    }
> +}
> +
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>  {
>      error_set(err, QERR_UNSUPPORTED);
> 

-- 
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 --]

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

On 03/12/2013 11:05 PM, Eric Blake wrote:
> On 03/12/2013 03:08 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-win32.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
>> +{
>> +    SYSTEMTIME ts;
>> +    FILETIME tf;
>> +    LONGLONG time;
>> +
>> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
>> +    if (error_is_set(errp)) {
>> +        error_setg(errp, "Failed to acquire privilege");
>> +        return;
>> +    }
> Earlier, you told me that acquire_privilege is auto-dropped after a
> successful SetSystemTime.  But here, you acquire the privilege...
>
>> +
>> +    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
>> +        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
>> +        return;
> ...then return early without ever relinquishing it.
>
>> +    }
>> +
>> +    time = time_ns / 100 + W32_FT_OFFSET;
>> +
>> +    tf.dwLowDateTime = (DWORD) time;
>> +    tf.dwHighDateTime = (DWORD) (time >> 32);
>> +
>> +    if (!FileTimeToSystemTime(&tf, &ts)) {
>> +        error_setg(errp, "Failed to convert system time");
>> +        return;
>> +    }
> I would reorder the acquire_privilege to here, to give us the best
> possible chance of avoiding a leak of privileges when the user passes
> bogus data.

It make sense, I should thought about this, thank you!

>> +
>> +    if (!SetSystemTime(&ts)) {
>> +        slog("guest-set-time failed: %d", GetLastError());
>> +        error_setg_errno(errp, errno, "Failed to set time to guest");
>> +        return;
>> +    }
>> +}
>> +
>>   int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
>>   {
>>       error_set(err, QERR_UNSUPPORTED);
>>


-- 
Lei

[Qemu-devel] [PATCH 0/2 v2] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
Now It was just tested on Windows XP SP3. I planed to test
it on Windows 7 today, unfortunately did not find the ISO,
so I have to download it. Since it's still on going, I'd like
to send this series out to have your suggestions. Please
comment!

Thanks.


Changes since v1:
  - Make the macro for the offset between windows baseline
    and Unix Epoch more readable from Eric.
  - Overflow check for filetime pointed by Eric.

Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0a2bb34..b5d4bd3 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -140,6 +140,41 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    SYSTEMTIME ts;
+    FILETIME tf;
+    LONGLONG time;
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        error_setg(errp, "Failed to acquire privilege");
+        return;
+    }
+
+    time = time_ns / 100 + W32_FT_OFFSET;
+    /* FILETIME values should be less than 0x8000000000000000
+       for function FileTimeToSystemTime. */
+    if (time & 0x8000000000000000) {
+        error_setg(errp, "Time %" PRId64 "is too large", time);
+        return;
+    }
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        slog("guest-set-time failed: %d", GetLastError());
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        return;
+    }
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Eric Blake]

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

On 03/08/2013 09:56 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 

> +
> +    time = time_ns / 100 + W32_FT_OFFSET;
> +    /* FILETIME values should be less than 0x8000000000000000
> +       for function FileTimeToSystemTime. */
> +    if (time & 0x8000000000000000) {
> +        error_setg(errp, "Time %" PRId64 "is too large", time);
> +        return;
> +    }

That doesn't cover all cases of overflow.  Better would be:

if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
    error ...
}
time = time_ns / 100 + W32_FT_OFFSET;

-- 
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 --]

[Qemu-devel] [PATCH 0/2] Add Windows support for time resync by qemu-ga

[Lei Li]

This patch series attempts to add Windows implementation
for qemu-ga commands guest-get-time and guest-set-time.

The previous thread about the interfaces introduced and 
the POSIX-specific command implementation has already
been accepted, the reference link:

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

Notes:
For now, It was just tested on Windows XP SP3. I will test
it on Windows 7 later. As I am new to windows development,
so there may be a lot of flaws. Please let me know your
suggestions, and comments are very welcome!

Thanks.


Lei Li (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time

[Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-win32.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 4febec7..1a90aa7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -136,6 +136,41 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    SYSTEMTIME ts;
+    FILETIME tf;
+    LONGLONG 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;
+    }
+
+    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
+    if (error_is_set(errp)) {
+        error_setg(errp, "Failed to acquire privilege");
+        return;
+    }
+
+    time = time_ns / 100 + _W32_FT_OFFSET;
+
+    tf.dwLowDateTime = (DWORD) time;
+    tf.dwHighDateTime = (DWORD) (time >> 32);
+
+    if (!FileTimeToSystemTime(&tf, &ts)) {
+        error_setg(errp, "Failed to convert system time");
+        return;
+    }
+
+    if (!SetSystemTime(&ts)) {
+        slog("guest-set-time failed: %d", GetLastError());
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        return;
+    }   
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Eric Blake]

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

On 03/06/2013 06:45 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 4febec7..1a90aa7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -136,6 +136,41 @@ int64_t qmp_guest_get_time(Error **errp)
>     return time_ns;
>  }
>  
> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +{
> +    SYSTEMTIME ts;
> +    FILETIME tf;
> +    LONGLONG 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;
> +    }

Do you really need this?  That is, don't you already know what size
time_t is on windows; not to mention that on Windows, you aren't going
through time_t, but directly to tf.dwHighDateTime.

> +
> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> +    if (error_is_set(errp)) {
> +        error_setg(errp, "Failed to acquire privilege");
> +        return;
> +    }
> +
> +    time = time_ns / 100 + _W32_FT_OFFSET;

On the other hand, _this_ operation can overflow, so you should be
checking that time_ns doesn't result in an unexpected time value.

> +
> +    tf.dwLowDateTime = (DWORD) time;
> +    tf.dwHighDateTime = (DWORD) (time >> 32);
> +
> +    if (!FileTimeToSystemTime(&tf, &ts)) {
> +        error_setg(errp, "Failed to convert system time");
> +        return;
> +    }
> +
> +    if (!SetSystemTime(&ts)) {
> +        slog("guest-set-time failed: %d", GetLastError());
> +        error_setg_errno(errp, errno, "Failed to set time to guest");
> +        return;
> +    }   

Trailing whitespace.  Run your submission through scripts/checkpatch.pl.

Should you relinquish the SE_SYSTEMTIME_NAME privilege when exiting this
function, instead of leaving it always enabled for the remaining life of
the agent service?

-- 
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 --]

Re: [Qemu-devel] [PATCH 2/2] qga: add windows implementation for guest-set-time

[Lei Li]

On 03/06/2013 11:05 PM, Eric Blake wrote:
> On 03/06/2013 06:45 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-win32.c |   35 +++++++++++++++++++++++++++++++++++
>>   1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 4febec7..1a90aa7 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -136,6 +136,41 @@ int64_t qmp_guest_get_time(Error **errp)
>>      return time_ns;
>>   }
>>   
>> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
>> +{
>> +    SYSTEMTIME ts;
>> +    FILETIME tf;
>> +    LONGLONG 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;
>> +    }
> Do you really need this?  That is, don't you already know what size
> time_t is on windows; not to mention that on Windows, you aren't going
> through time_t, but directly to tf.dwHighDateTime.

You are right.

>
>> +
>> +    acquire_privilege(SE_SYSTEMTIME_NAME, errp);
>> +    if (error_is_set(errp)) {
>> +        error_setg(errp, "Failed to acquire privilege");
>> +        return;
>> +    }
>> +
>> +    time = time_ns / 100 + _W32_FT_OFFSET;
> On the other hand, _this_ operation can overflow, so you should be
> checking that time_ns doesn't result in an unexpected time value.

OK.

>> +
>> +    tf.dwLowDateTime = (DWORD) time;
>> +    tf.dwHighDateTime = (DWORD) (time >> 32);
>> +
>> +    if (!FileTimeToSystemTime(&tf, &ts)) {
>> +        error_setg(errp, "Failed to convert system time");
>> +        return;
>> +    }
>> +
>> +    if (!SetSystemTime(&ts)) {
>> +        slog("guest-set-time failed: %d", GetLastError());
>> +        error_setg_errno(errp, errno, "Failed to set time to guest");
>> +        return;
>> +    }
> Trailing whitespace.  Run your submission through scripts/checkpatch.pl.

Yes.

> Should you relinquish the SE_SYSTEMTIME_NAME privilege when exiting this
> function, instead of leaving it always enabled for the remaining life of
> the agent service?

According to the remarks on msdn, this SetSystemTime function will
disables the SE_SYSTEMTIME_NAME privilege before returning.



-- 
Lei