Re: [Qemu-devel] [PATCH 1/3] qga: add support to get host time

[Eric Blake]

On 01/06/2013 03:06 AM, Lei Li wrote:
> Signed-off-by: Lei Li <address@hidden>
> ---
>  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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature