Patchwork [1/3] qga: add support to get host time

login
register
mail settings
Submitter Lei Li
Date Jan. 6, 2013, 10:06 a.m.
Message ID <1357466820-12860-2-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/209731/
State New
Headers show

Comments

Lei Li - Jan. 6, 2013, 10:06 a.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   18 ++++++++++++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)
Eric Blake - Jan. 7, 2013, 9:52 p.m.
On 01/06/2013 03:06 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
>  
> +static HostTimeInfo *get_host_time(void)
> +{

> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;

Why usec?  struct timespec with nanoseconds might be a nicer unit, even
if for the initial implementation, you use
qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime
library for full ns resolution.  If nothing else, the lesson that ought
to be learned from the proliferation of time types is that any time you
don't report lots of precision, someone comes along later on having to
add yet another interface adding more precision.


> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
>  
>  ##
> +# @HostTimeInfo
> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.

Document that this is relative to the Epoch of 1970-01-01 (no matter
what the host uses for its internal reference point).

> +#
> +# @microseconds: "microseconds" time from the host.

Again, nanoseconds (struct timespec) might be nicer.

> +#
> +# @utc-offset: information about utc offset.

In what format?  Minutes away from UTC, a 4-digit decimal value, or
something else (that is, is a one-hour offset represented as 60 or 100)?
 Are negative values east or west of UTC?

> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }

Indentation seems inconsistent.

Ah, here you made them mandatory - only your cover letter implied that
they were optional.
Luiz Capitulino - Jan. 9, 2013, 1:32 p.m.
On Sun,  6 Jan 2013 18:06:58 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
>  
> +static HostTimeInfo *get_host_time(void)
> +{

Does this build? Because no one is using this function.

> +    int err;
> +    qemu_timeval tq;
> +    HostTimeInfo *host_time;
> +
> +    err = qemu_gettimeofday(&tq);
> +    if (err < 0) {

I'd recommend taking an Error * argument and setting it with
error_set_errno().

> +        return NULL;
> +    }
> +
> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index ed0eb69..7793aff 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
>  
>  ##
> +# @HostTimeInfo

I'm a bit confused, why do you call it HostTimeInfo if this runs
in the guest?

> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.
> +#
> +# @microseconds: "microseconds" time from the host.
> +#
> +# @utc-offset: information about utc offset.
> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
Michael Roth - Jan. 9, 2013, 3:36 p.m.
On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   18 ++++++++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a657201..26b0fa0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -91,6 +91,24 @@ exit_err:
>      error_set(err, QERR_UNDEFINED_ERROR);
>  }
> 
> +static HostTimeInfo *get_host_time(void)
> +{

Should squash this into patch #2, since it doesn't get used
till then. Otherwise we'll break build bisection when compiling
with -Wunused/-Wall && -Werror, which is usually the default.

> +    int err;
> +    qemu_timeval tq;
> +    HostTimeInfo *host_time;
> +
> +    err = qemu_gettimeofday(&tq);
> +    if (err < 0) {
> +        return NULL;
> +    }
> +
> +    host_time = g_malloc0(sizeof(HostTimeInfo));
> +    host_time->seconds = tq.tv_sec;
> +    host_time->microseconds = tq.tv_usec;
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index ed0eb69..7793aff 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,23 @@
>  { 'command': 'guest-ping' }
> 
>  ##
> +# @HostTimeInfo
> +#
> +# Information about host time.
> +#
> +# @seconds: "seconds" time from the host.
> +#
> +# @microseconds: "microseconds" time from the host.
> +#
> +# @utc-offset: information about utc offset.
> +#
> +# Since: 1.4
> +##
> +{ 'type': 'HostTimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +             'utc-offset': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
> -- 
> 1.7.7.6
>
Lei Li - Jan. 11, 2013, 7:18 a.m.
On 01/08/2013 05:52 AM, Eric Blake wrote:
> On 01/06/2013 03:06 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>   
>> +static HostTimeInfo *get_host_time(void)
>> +{
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
> Why usec?  struct timespec with nanoseconds might be a nicer unit, even
> if for the initial implementation, you use
> qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime
> library for full ns resolution.  If nothing else, the lesson that ought
> to be learned from the proliferation of time types is that any time you
> don't report lots of precision, someone comes along later on having to
> add yet another interface adding more precision.

ok, I will have a try to change it to a ns resolution.

>
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>   
>>   ##
>> +# @HostTimeInfo
>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
> Document that this is relative to the Epoch of 1970-01-01 (no matter
> what the host uses for its internal reference point).

Sure.

>
>> +#
>> +# @microseconds: "microseconds" time from the host.
> Again, nanoseconds (struct timespec) might be nicer.
>
>> +#
>> +# @utc-offset: information about utc offset.
> In what format?  Minutes away from UTC, a 4-digit decimal value, or
> something else (that is, is a one-hour offset represented as 60 or 100)?
>   Are negative values east or west of UTC?

For this version, it's a one-hour offset represented as:±[hh].
Negative values are west, andpositive values are east of UTC.

>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
> Indentation seems inconsistent.
>
> Ah, here you made them mandatory - only your cover letter implied that
> they were optional.

Sorry for the inconsistent indentation.
No, this is just the definition of the time structure. The optional arguments
mentioned in the cover letter are for command "guest-set-time". :)
Lei Li - Jan. 11, 2013, 7:19 a.m.
On 01/09/2013 09:32 PM, Luiz Capitulino wrote:
> On Sun,  6 Jan 2013 18:06:58 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>   
>> +static HostTimeInfo *get_host_time(void)
>> +{
> Does this build? Because no one is using this function.

Yes, this should be squashed into patch #2 as Mike also
pointed out that.

>> +    int err;
>> +    qemu_timeval tq;
>> +    HostTimeInfo *host_time;
>> +
>> +    err = qemu_gettimeofday(&tq);
>> +    if (err < 0) {
> I'd recommend taking an Error * argument and setting it with
> error_set_errno().

ok.

>
>> +        return NULL;
>> +    }
>> +
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index ed0eb69..7793aff 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>   
>>   ##
>> +# @HostTimeInfo
> I'm a bit confused, why do you call it HostTimeInfo if this runs
> in the guest?

I call it HostTimeInfo because it contains the host time information.
But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo'
might be better?

>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
>> +#
>> +# @microseconds: "microseconds" time from the host.
>> +#
>> +# @utc-offset: information about utc offset.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
Lei Li - Jan. 11, 2013, 7:20 a.m.
On 01/09/2013 11:36 PM, mdroth wrote:
> On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   18 ++++++++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a657201..26b0fa0 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -91,6 +91,24 @@ exit_err:
>>       error_set(err, QERR_UNDEFINED_ERROR);
>>   }
>>
>> +static HostTimeInfo *get_host_time(void)
>> +{
> Should squash this into patch #2, since it doesn't get used
> till then. Otherwise we'll break build bisection when compiling
> with -Wunused/-Wall && -Werror, which is usually the default.

ok, got it.
Thanks!

>> +    int err;
>> +    qemu_timeval tq;
>> +    HostTimeInfo *host_time;
>> +
>> +    err = qemu_gettimeofday(&tq);
>> +    if (err < 0) {
>> +        return NULL;
>> +    }
>> +
>> +    host_time = g_malloc0(sizeof(HostTimeInfo));
>> +    host_time->seconds = tq.tv_sec;
>> +    host_time->microseconds = tq.tv_usec;
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index ed0eb69..7793aff 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,23 @@
>>   { 'command': 'guest-ping' }
>>
>>   ##
>> +# @HostTimeInfo
>> +#
>> +# Information about host time.
>> +#
>> +# @seconds: "seconds" time from the host.
>> +#
>> +# @microseconds: "microseconds" time from the host.
>> +#
>> +# @utc-offset: information about utc offset.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'HostTimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +             'utc-offset': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
>> -- 
>> 1.7.7.6
>>
Luiz Capitulino - Jan. 11, 2013, 11:14 a.m.
On Fri, 11 Jan 2013 15:19:35 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> >>   ##
> >> +# @HostTimeInfo
> > I'm a bit confused, why do you call it HostTimeInfo if this runs
> > in the guest?
> 
> I call it HostTimeInfo because it contains the host time information.

qemu-ga runs in the guest, so it's actually the guest time.

> But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo'
> might be better?

Yes, looks better for me.
Eric Blake - Jan. 11, 2013, 3:37 p.m.
On 01/11/2013 12:18 AM, Lei Li wrote:
> 
> For this version, it's a one-hour offset represented as:±[hh].
> Negative values are west, andpositive values are east of UTC.

Won't work.  There are timezones with a half-hour offset.  You need to
express offset at least in terms of minutes, not just hours.
Lei Li - Jan. 14, 2013, 3:17 a.m.
On 01/11/2013 11:37 PM, Eric Blake wrote:
> On 01/11/2013 12:18 AM, Lei Li wrote:
>> For this version, it's a one-hour offset represented as:±[hh].
>> Negative values are west, andpositive values are east of UTC.
> Won't work.  There are timezones with a half-hour offset.  You need to
> express offset at least in terms of minutes, not just hours.
>
Yes, I have thought about this. For the timezones with a half-hour offset,
it can just represented as +0.5 for example.
Eric Blake - Jan. 14, 2013, 2:23 p.m.
On 01/13/2013 08:17 PM, Lei Li wrote:
> On 01/11/2013 11:37 PM, Eric Blake wrote:
>> On 01/11/2013 12:18 AM, Lei Li wrote:
>>> For this version, it's a one-hour offset represented as:±[hh].
>>> Negative values are west, andpositive values are east of UTC.
>> Won't work.  There are timezones with a half-hour offset.  You need to
>> express offset at least in terms of minutes, not just hours.
>>
> Yes, I have thought about this. For the timezones with a half-hour offset,
> it can just represented as +0.5 for example.

Floating point parsing is a pain compared to integer parsing.  Remember,
there is no way in POSIX to print a locale-independent floating point
number, which forces programs to jump through quite a few hoops to
guarantee that they are using '.' for the radix character in JSON output
while still honoring the user's locale.

Please, use something based at a minimum on minutes (seconds is also
possible), not hours,

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a657201..26b0fa0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -91,6 +91,24 @@  exit_err:
     error_set(err, QERR_UNDEFINED_ERROR);
 }
 
+static HostTimeInfo *get_host_time(void)
+{
+    int err;
+    qemu_timeval tq;
+    HostTimeInfo *host_time;
+
+    err = qemu_gettimeofday(&tq);
+    if (err < 0) {
+        return NULL;
+    }
+
+    host_time = g_malloc0(sizeof(HostTimeInfo));
+    host_time->seconds = tq.tv_sec;
+    host_time->microseconds = tq.tv_usec;
+
+    return host_time;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index ed0eb69..7793aff 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -83,6 +83,23 @@ 
 { 'command': 'guest-ping' }
 
 ##
+# @HostTimeInfo
+#
+# Information about host time.
+#
+# @seconds: "seconds" time from the host.
+#
+# @microseconds: "microseconds" time from the host.
+#
+# @utc-offset: information about utc offset.
+#
+# Since: 1.4
+##
+{ 'type': 'HostTimeInfo',
+  'data': { 'seconds': 'int', 'microseconds': 'int',
+             'utc-offset': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.