[Qemu-devel] [PATCH 1/2] qga: add windows implementation for guest-get-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 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 |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..0a2bb34 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,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: 579 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 |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 

> +
> +   time_ns = (int64_t)((time.ns100 - W32_FT_OFFSET) * 100);

This could overflow, but only so many years into the future that it's
beyond our lifetime, so I don't mind leaving it as is.

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

-- 
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 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 | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..0a2bb34 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,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.11.7

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

[lilei]

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

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..0a2bb34 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,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

[mdroth]

On Wed, Mar 13, 2013 at 06:10:30PM +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 |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 7e8ecb3..0a2bb34 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,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));

Don't we need to free this at some point?

> +   int64_t time_ns;
> +   union {
> +       UINT64 ns100;
> +       FILETIME tf;
> +   } time;
> +
> +   GetSystemTime(ts);
> +   if (!ts) {
> +       slog("guest-get-time failed: %d", GetLastError());

GetSystemTime() does not seem to set an error that can be retrieved by
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);

I'm not sure how safe this union stuff is. The documentation suggests that in
some circumstances the low/high fields in FILETIME might be padded for
64-bit alignment and that doing this type of cast could generate an
alignment fault:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms724284(v=vs.85).aspx

or it might, perhaps even worse, just silently report the wrong time.

I think we should just do the math explicitly:

(((tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100)

(or something along that line)

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

[mdroth]

On Wed, Mar 13, 2013 at 03:07:52PM -0500, mdroth wrote:
> On Wed, Mar 13, 2013 at 06:10:30PM +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 |   32 ++++++++++++++++++++++++++++++++
> >  1 files changed, 32 insertions(+), 0 deletions(-)
> > 
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 7e8ecb3..0a2bb34 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,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));
> 
> Don't we need to free this at some point?

Actually, do we even need malloc? Can't be just allocate SYSTEMTIME
struct on the stack and pass a pointer to GetSystemTime?

> 
> > +   int64_t time_ns;
> > +   union {
> > +       UINT64 ns100;
> > +       FILETIME tf;
> > +   } time;
> > +
> > +   GetSystemTime(ts);
> > +   if (!ts) {
> > +       slog("guest-get-time failed: %d", GetLastError());
> 
> GetSystemTime() does not seem to set an error that can be retrieved by
> 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);
> 
> I'm not sure how safe this union stuff is. The documentation suggests that in
> some circumstances the low/high fields in FILETIME might be padded for
> 64-bit alignment and that doing this type of cast could generate an
> alignment fault:
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms724284(v=vs.85).aspx
> 
> or it might, perhaps even worse, just silently report the wrong time.
> 
> I think we should just do the math explicitly:
> 
> (((tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100)
> 
> (or something along that line)
> 
> > +
> > +   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

[Lei Li]

On 03/14/2013 04:07 AM, mdroth wrote:
> On Wed, Mar 13, 2013 at 06:10:30PM +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 |   32 ++++++++++++++++++++++++++++++++
>>   1 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 7e8ecb3..0a2bb34 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,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));
> Don't we need to free this at some point?
>
Sorry, my fault here...

>> +   int64_t time_ns;
>> +   union {
>> +       UINT64 ns100;
>> +       FILETIME tf;
>> +   } time;
>> +
>> +   GetSystemTime(ts);
>> +   if (!ts) {
>> +       slog("guest-get-time failed: %d", GetLastError());
> GetSystemTime() does not seem to set an error that can be retrieved by
> GetLastError().

Yes, you are right.

>> +       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);
> I'm not sure how safe this union stuff is. The documentation suggests that in
> some circumstances the low/high fields in FILETIME might be padded for
> 64-bit alignment and that doing this type of cast could generate an
> alignment fault:
>
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms724284(v=vs.85).aspx
>
> or it might, perhaps even worse, just silently report the wrong time.
>
> I think we should just do the math explicitly:
>
> (((tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100)
>
> (or something along that line)

OK, thanks.

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


-- 
Lei

[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 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 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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..b395bd5 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,29 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     }
 }
 
+int64_t qmp_guest_get_time(Error **errp)
+{
+    SYSTEMTIME ts = {0};
+    int64_t time_ns;
+    FILETIME tf;
+
+    GetSystemTime(&ts);
+    if (ts.wYear < 1601 || ts.wYear > 30827) {
+        error_setg(errp, "Failed to get time");
+        return -1;
+    }
+
+    if (!SystemTimeToFileTime(&ts, &tf)) {
+        error_setg_errno(errp, errno, "Failed to convert system time");
+        return -1;
+    }
+
+    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
+                - 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.11.7

[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

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

[mdroth]

On Thu, Mar 14, 2013 at 11:05:52PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-win32.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 7e8ecb3..b395bd5 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,29 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>      }
>  }
> 
> +int64_t qmp_guest_get_time(Error **errp)
> +{
> +    SYSTEMTIME ts = {0};
> +    int64_t time_ns;
> +    FILETIME tf;
> +
> +    GetSystemTime(&ts);
> +    if (ts.wYear < 1601 || ts.wYear > 30827) {
> +        error_setg(errp, "Failed to get time");
> +        return -1;
> +    }
> +
> +    if (!SystemTimeToFileTime(&ts, &tf)) {
> +        error_setg_errno(errp, errno, "Failed to convert system time");

I missed this is well, SystemTimeToFileTime() doesn't set errno, but if
this is the only change needed I can fix this up in my tree.

> +        return -1;
> +    }
> +
> +    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> +                - 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.11.7
> 

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

[mdroth]

On Thu, Mar 14, 2013 at 11:05:51PM +0800, Lei Li wrote:
> 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!

Series looks good other than comment in patch 1. I can fix this in tree
or you can send another version.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

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

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

[Lei Li]

On 03/15/2013 05:41 AM, mdroth wrote:
> On Thu, Mar 14, 2013 at 11:05:51PM +0800, Lei Li wrote:
>> 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!
> Series looks good other than comment in patch 1. I can fix this in tree
> or you can send another version.

I am very sorry for this, I should check the rest of it...  :-(
Sure, I will submit new version with this fixed.

Thanks for your time!

>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
>> 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
>>


-- 
Lei

[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 | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b19be9d..d98e3ee 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;
@@ -280,8 +286,25 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **err)
 
 int64_t qmp_guest_get_time(Error **errp)
 {
-    error_set(errp, QERR_UNSUPPORTED);
-    return -1;
+    SYSTEMTIME ts = {0};
+    int64_t time_ns;
+    FILETIME tf;
+
+    GetSystemTime(&ts);
+    if (ts.wYear < 1601 || ts.wYear > 30827) {
+        error_setg(errp, "Failed to get time");
+        return -1;
+    }
+
+    if (!SystemTimeToFileTime(&ts, &tf)) {
+        error_setg(errp, "Failed to convert system time: %d", (int)GetLastError());
+        return -1;
+    }
+
+    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
+                - W32_FT_OFFSET) * 100;
+
+    return time_ns;
 }
 
 void qmp_guest_set_time(int64_t time_ns, Error **errp)
-- 
1.7.11.7