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

[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 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 |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..4febec7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -22,6 +22,8 @@
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
 #endif
 
+#define _W32_FT_OFFSET (116444736000000000ULL)
+
 static void acquire_privilege(const char *name, Error **err)
 {
     HANDLE token;
@@ -108,6 +110,32 @@ 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;
+   union {
+       UINT64 ns100;
+       FILETIME tf;
+   } time;
+
+   GetSystemTime(ts);
+   if (!ts) {
+       slog("guest-get-time failed: %d", GetLastError());
+       error_setg_errno(errp, errno, "Failed to get time");
+       return -1;
+   }
+
+   if (!SystemTimeToFileTime(ts, &time.tf)) {
+       error_setg_errno(errp, errno, "Failed to convert system time");
+       return -1;
+   }
+
+   time_ns = (int64_t)((time.ns100 - _W32_FT_OFFSET) * 100);
+
+   return time_ns;
+}
+
 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 1/2] qga: add windows implementation for guest-get-time

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1248 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 |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 7e8ecb3..4febec7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -22,6 +22,8 @@
>  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>  #endif
>  
> +#define _W32_FT_OFFSET (116444736000000000ULL)

Defining a macro with a leading underscore infringes on the namespace
reserved to the system headers and compiler implementation.  Drop the
leading underscore.

As written, the () are redundant.  However, it would be nicer to state
HOW you came up with this number (and not that you just did a google
search for it), as in:

/* 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))

-- 
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 1/2] qga: add windows implementation for guest-get-time

[Lei Li]

On 03/06/2013 11:31 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 |   28 ++++++++++++++++++++++++++++
>>   1 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 7e8ecb3..4febec7 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -22,6 +22,8 @@
>>   #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>>   #endif
>>   
>> +#define _W32_FT_OFFSET (116444736000000000ULL)
> Defining a macro with a leading underscore infringes on the namespace
> reserved to the system headers and compiler implementation.  Drop the
> leading underscore.
>
> As written, the () are redundant.  However, it would be nicer to state
> HOW you came up with this number (and not that you just did a google
> search for it), as in:
>
> /* 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))
>
Yes, it make sense, thanks!



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

[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 Lii (2):
    qga: add windows implementation for guest-get-time
    qga: add windows implementation for guest-set-time