Patchwork [1/2] qga: add guest-get-time command

login
register
mail settings
Submitter Lei Li
Date Jan. 27, 2013, 6:14 p.m.
Message ID <1359310460-23564-2-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/216025/
State New
Headers show

Comments

Lei Li - Jan. 27, 2013, 6:14 p.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/qapi/qmp/qerror.h |    3 +++
 qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
 qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 0 deletions(-)
Eric Blake - Jan. 28, 2013, 6:29 p.m.
On 01/27/2013 11:14 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/qapi/qmp/qerror.h |    3 +++
>  qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
>  qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+), 0 deletions(-)
> 
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 6c0a18d..0baf1a4 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -129,6 +129,9 @@ void assert_no_error(Error *err);
>  #define QERR_FEATURE_DISABLED \
>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>  
> +#define QERR_GET_TIME_FAILED \
> +    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
> +

These days, you shouldn't be defining a new QERR wrapper.  Instead,...[1]

>  
> +static TimeInfo *get_host_time(Error **errp)

This name is unusual...[2]

> +{
> +    int ret;
> +    qemu_timeval tq;
> +    TimeInfo *time_info;
> +
> +    ret = qemu_gettimeofday(&tq);
> +    if (ret < 0) {
> +        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);

[1]...use the right idiom here:

error_setg_errno(errp, errno, "Failed to get time");

> +        return NULL;
> +    }
> +
> +    time_info = g_malloc0(sizeof(TimeInfo));
> +    time_info->seconds = tq.tv_sec;
> +    time_info->microseconds = tq.tv_usec;

Is microseconds the right unit to expose through JSON, or are we going
to wish we had exposed nanoseconds down the road (you can still use
qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
time_info->nanoseconds, if we desire the extra granularity).

You aren't setting time_info->utc_offset.  Is that intentional?

> +
> +    return time_info;
> +}
> +
> +struct TimeInfo *qmp_guest_get_time(Error **errp)
> +{
> +    TimeInfo *time_info = get_host_time(errp);

[2]...given that it is only ever called from qmp_guest_get_time.

> +
> +    if (!time_info) {
> +        return NULL;
> +    }

These three lines can be deleted,

> +
> +    return time_info;

since this line will do the same thing if you let NULL time_info get
this far.

> +++ b/qga/qapi-schema.json
> @@ -83,6 +83,44 @@
>  { 'command': 'guest-ping' }
>  
>  ##
> +# @TimeInfo
> +#
> +# Time Information. It is relative to the Epoch of 1970-01-01.
> +#
> +# @seconds: "seconds" time unit.
> +#
> +# @microseconds: "microseconds" time unit.
> +#
> +# @utc-offset: Information about utc offset. Represented as:
> +#              ±[mmmm] based at a minimum on minutes, with

s/based at a minimum on//

This still doesn't state whether two hours east of UTC is represented as
120 or as 0200.

> +#              negative values are west and positive values
> +#              are east of UTC. The bounds of @utc-offset is
> +#              at most 24 hours away from UTC.
> +#
> +# Since: 1.4
> +##
> +{ 'type': 'TimeInfo',
> +  'data': { 'seconds': 'int', 'microseconds': 'int',
> +            'utc-offset': 'int' } }
> +
> +##
> +# @guest-get-time:
> +#
> +# Get the information about host time in UTC and the

s/host/guest/

> +# UTC offset.
> +#
> +# This command tries to get the host time which is
> +# presumably correct, since we need to be able to
> +# resynchronize guest clock to host's in guest.

This sentence doesn't make sense.  Better would be:

Get the guest's notion of the current time.

> +#
> +# Returns: @TimeInfo on success.
> +#
> +# Since 1.4
> +##
> +{ 'command': 'guest-get-time',
> +  'returns': 'TimeInfo' }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
>
Anthony Liguori - Jan. 28, 2013, 8:24 p.m.
Eric Blake <eblake@redhat.com> writes:

> On 01/27/2013 11:14 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>  include/qapi/qmp/qerror.h |    3 +++
>>  qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
>>  qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 71 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 6c0a18d..0baf1a4 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -129,6 +129,9 @@ void assert_no_error(Error *err);
>>  #define QERR_FEATURE_DISABLED \
>>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>>  
>> +#define QERR_GET_TIME_FAILED \
>> +    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
>> +
>
> These days, you shouldn't be defining a new QERR wrapper.  Instead,...[1]
>
>>  
>> +static TimeInfo *get_host_time(Error **errp)
>
> This name is unusual...[2]
>
>> +{
>> +    int ret;
>> +    qemu_timeval tq;
>> +    TimeInfo *time_info;
>> +
>> +    ret = qemu_gettimeofday(&tq);
>> +    if (ret < 0) {
>> +        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
>
> [1]...use the right idiom here:
>
> error_setg_errno(errp, errno, "Failed to get time");
>
>> +        return NULL;
>> +    }
>> +
>> +    time_info = g_malloc0(sizeof(TimeInfo));
>> +    time_info->seconds = tq.tv_sec;
>> +    time_info->microseconds = tq.tv_usec;
>
> Is microseconds the right unit to expose through JSON, or are we going
> to wish we had exposed nanoseconds down the road (you can still use
> qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
> time_info->nanoseconds, if we desire the extra granularity).
>
> You aren't setting time_info->utc_offset.  Is that intentional?

Moreover, there's no need to specify microseconds and seconds.  A 64-bit
value for nanoseconds is sufficient.  A signed value can represent
1678 -> 2262.  If anyone is using qemu-ga in 2262, they'll have to
introduce a new command for their quantum emulators :-)

Setting time independent of date is a bit silly because time cannot be
interpreted correctly without a date.

A single value of nanoseconds since the epoch (interpreted as UTC/GMT
time) is really the best strategy.  The host shouldn't be involved in
guest time zones.  In a Cloud environment, it's pretty normal to have
different guests using different time zones.

Regards,

Anthony Liguori

>
>> +
>> +    return time_info;
>> +}
>> +
>> +struct TimeInfo *qmp_guest_get_time(Error **errp)
>> +{
>> +    TimeInfo *time_info = get_host_time(errp);
>
> [2]...given that it is only ever called from qmp_guest_get_time.
>
>> +
>> +    if (!time_info) {
>> +        return NULL;
>> +    }
>
> These three lines can be deleted,
>
>> +
>> +    return time_info;
>
> since this line will do the same thing if you let NULL time_info get
> this far.
>
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,44 @@
>>  { 'command': 'guest-ping' }
>>  
>>  ##
>> +# @TimeInfo
>> +#
>> +# Time Information. It is relative to the Epoch of 1970-01-01.
>> +#
>> +# @seconds: "seconds" time unit.
>> +#
>> +# @microseconds: "microseconds" time unit.
>> +#
>> +# @utc-offset: Information about utc offset. Represented as:
>> +#              ±[mmmm] based at a minimum on minutes, with
>
> s/based at a minimum on//
>
> This still doesn't state whether two hours east of UTC is represented as
> 120 or as 0200.
>
>> +#              negative values are west and positive values
>> +#              are east of UTC. The bounds of @utc-offset is
>> +#              at most 24 hours away from UTC.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'TimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +            'utc-offset': 'int' } }
>> +
>> +##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
>
> s/host/guest/
>
>> +# UTC offset.
>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since we need to be able to
>> +# resynchronize guest clock to host's in guest.
>
> This sentence doesn't make sense.  Better would be:
>
> Get the guest's notion of the current time.
>
>> +#
>> +# Returns: @TimeInfo on success.
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'guest-get-time',
>> +  'returns': 'TimeInfo' }
>> +
>> +##
>>  # @GuestAgentCommandInfo:
>>  #
>>  # Information about guest agent commands.
>> 
>
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Lei Li - Jan. 30, 2013, 7:21 a.m.
On 01/29/2013 02:29 AM, Eric Blake wrote:
> On 01/27/2013 11:14 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   include/qapi/qmp/qerror.h |    3 +++
>>   qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
>>   qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 71 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 6c0a18d..0baf1a4 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -129,6 +129,9 @@ void assert_no_error(Error *err);
>>   #define QERR_FEATURE_DISABLED \
>>       ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>>   
>> +#define QERR_GET_TIME_FAILED \
>> +    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
>> +
> These days, you shouldn't be defining a new QERR wrapper.  Instead,...[1]
>
>>   
>> +static TimeInfo *get_host_time(Error **errp)
> This name is unusual...[2]

Okay, I think there might be misunderstanding here.

The current implementation of this 'guest-get-time'
command is trying to get the*  *_host_  time, since the guest
time might be not correct. This qemu_gettimeofday should
return the host time, or am I wrong?

>> +{
>> +    int ret;
>> +    qemu_timeval tq;
>> +    TimeInfo *time_info;
>> +
>> +    ret = qemu_gettimeofday(&tq);
>> +    if (ret < 0) {
>> +        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
> [1]...use the right idiom here:
>
> error_setg_errno(errp, errno, "Failed to get time");
>
>> +        return NULL;
>> +    }
>> +
>> +    time_info = g_malloc0(sizeof(TimeInfo));
>> +    time_info->seconds = tq.tv_sec;
>> +    time_info->microseconds = tq.tv_usec;
> Is microseconds the right unit to expose through JSON, or are we going
> to wish we had exposed nanoseconds down the road (you can still use
> qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
> time_info->nanoseconds, if we desire the extra granularity).
>
> You aren't setting time_info->utc_offset.  Is that intentional?

Yes, since 'guest-get-time' is supposed to get host time in UTC, the utc_offset
is zero. When guest want to set different time zone it can set the
utc_offset through 'guest-set-time'.

>> +
>> +    return time_info;
>> +}
>> +
>> +struct TimeInfo *qmp_guest_get_time(Error **errp)
>> +{
>> +    TimeInfo *time_info = get_host_time(errp);
> [2]...given that it is only ever called from qmp_guest_get_time.
>
>> +
>> +    if (!time_info) {
>> +        return NULL;
>> +    }
> These three lines can be deleted,
>
>> +
>> +    return time_info;
> since this line will do the same thing if you let NULL time_info get
> this far.
>
>> +++ b/qga/qapi-schema.json
>> @@ -83,6 +83,44 @@
>>   { 'command': 'guest-ping' }
>>   
>>   ##
>> +# @TimeInfo
>> +#
>> +# Time Information. It is relative to the Epoch of 1970-01-01.
>> +#
>> +# @seconds: "seconds" time unit.
>> +#
>> +# @microseconds: "microseconds" time unit.
>> +#
>> +# @utc-offset: Information about utc offset. Represented as:
>> +#              ±[mmmm] based at a minimum on minutes, with
> s/based at a minimum on//
>
> This still doesn't state whether two hours east of UTC is represented as
> 120 or as 0200.
>
>> +#              negative values are west and positive values
>> +#              are east of UTC. The bounds of @utc-offset is
>> +#              at most 24 hours away from UTC.
>> +#
>> +# Since: 1.4
>> +##
>> +{ 'type': 'TimeInfo',
>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>> +            'utc-offset': 'int' } }
>> +
>> +##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
> s/host/guest/
>
>> +# UTC offset.
>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since we need to be able to
>> +# resynchronize guest clock to host's in guest.
> This sentence doesn't make sense.  Better would be:
>
> Get the guest's notion of the current time.
>
>> +#
>> +# Returns: @TimeInfo on success.
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'guest-get-time',
>> +  'returns': 'TimeInfo' }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
>>
Lei Li - Jan. 30, 2013, 7:37 a.m.
On 01/29/2013 04:24 AM, Anthony Liguori wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/27/2013 11:14 AM, Lei Li wrote:
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> ---
>>>   include/qapi/qmp/qerror.h |    3 +++
>>>   qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
>>>   qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 71 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>>> index 6c0a18d..0baf1a4 100644
>>> --- a/include/qapi/qmp/qerror.h
>>> +++ b/include/qapi/qmp/qerror.h
>>> @@ -129,6 +129,9 @@ void assert_no_error(Error *err);
>>>   #define QERR_FEATURE_DISABLED \
>>>       ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>>>   
>>> +#define QERR_GET_TIME_FAILED \
>>> +    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
>>> +
>> These days, you shouldn't be defining a new QERR wrapper.  Instead,...[1]
>>
>>>   
>>> +static TimeInfo *get_host_time(Error **errp)
>> This name is unusual...[2]
>>
>>> +{
>>> +    int ret;
>>> +    qemu_timeval tq;
>>> +    TimeInfo *time_info;
>>> +
>>> +    ret = qemu_gettimeofday(&tq);
>>> +    if (ret < 0) {
>>> +        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
>> [1]...use the right idiom here:
>>
>> error_setg_errno(errp, errno, "Failed to get time");
>>
>>> +        return NULL;
>>> +    }
>>> +
>>> +    time_info = g_malloc0(sizeof(TimeInfo));
>>> +    time_info->seconds = tq.tv_sec;
>>> +    time_info->microseconds = tq.tv_usec;
>> Is microseconds the right unit to expose through JSON, or are we going
>> to wish we had exposed nanoseconds down the road (you can still use
>> qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
>> time_info->nanoseconds, if we desire the extra granularity).
>>
>> You aren't setting time_info->utc_offset.  Is that intentional?
> Moreover, there's no need to specify microseconds and seconds.  A 64-bit
> value for nanoseconds is sufficient.  A signed value can represent
> 1678 -> 2262.  If anyone is using qemu-ga in 2262, they'll have to
> introduce a new command for their quantum emulators :-)
>
> Setting time independent of date is a bit silly because time cannot be
> interpreted correctly without a date.
>
> A single value of nanoseconds since the epoch (interpreted as UTC/GMT
> time) is really the best strategy.  The host shouldn't be involved in
> guest time zones.  In a Cloud environment, it's pretty normal to have
> different guests using different time zones.
>
> Regards,
>
> Anthony Liguori
>
>>> +
>>> +    return time_info;
>>> +}
>>> +
>>> +struct TimeInfo *qmp_guest_get_time(Error **errp)
>>> +{
>>> +    TimeInfo *time_info = get_host_time(errp);
>> [2]...given that it is only ever called from qmp_guest_get_time.
>>
>>> +
>>> +    if (!time_info) {
>>> +        return NULL;
>>> +    }
>> These three lines can be deleted,
>>
>>> +
>>> +    return time_info;
>> since this line will do the same thing if you let NULL time_info get
>> this far.
>>
>>> +++ b/qga/qapi-schema.json
>>> @@ -83,6 +83,44 @@
>>>   { 'command': 'guest-ping' }
>>>   
>>>   ##
>>> +# @TimeInfo
>>> +#
>>> +# Time Information. It is relative to the Epoch of 1970-01-01.
>>> +#
>>> +# @seconds: "seconds" time unit.
>>> +#
>>> +# @microseconds: "microseconds" time unit.
>>> +#
>>> +# @utc-offset: Information about utc offset. Represented as:
>>> +#              ±[mmmm] based at a minimum on minutes, with
>> s/based at a minimum on//
>>
>> This still doesn't state whether two hours east of UTC is represented as
>> 120 or as 0200.
>>
It should be 120.
Yeah, I should make it clear.

I am thinking if this 'utc_offset' can be made as a string, represented
like: ±[hh]:[mm], +08:45 (8 hours and 45 minutes) for example.

>>> +#              negative values are west and positive values
>>> +#              are east of UTC. The bounds of @utc-offset is
>>> +#              at most 24 hours away from UTC.
>>> +#
>>> +# Since: 1.4
>>> +##
>>> +{ 'type': 'TimeInfo',
>>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>>> +            'utc-offset': 'int' } }
>>> +
>>> +##
>>> +# @guest-get-time:
>>> +#
>>> +# Get the information about host time in UTC and the
>> s/host/guest/
>>
>>> +# UTC offset.
>>> +#
>>> +# This command tries to get the host time which is
>>> +# presumably correct, since we need to be able to
>>> +# resynchronize guest clock to host's in guest.
>> This sentence doesn't make sense.  Better would be:
>>
>> Get the guest's notion of the current time.
>>
>>> +#
>>> +# Returns: @TimeInfo on success.
>>> +#
>>> +# Since 1.4
>>> +##
>>> +{ 'command': 'guest-get-time',
>>> +  'returns': 'TimeInfo' }
>>> +
>>> +##
>>>   # @GuestAgentCommandInfo:
>>>   #
>>>   # Information about guest agent commands.
>>>
>> -- 
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
Lei Li - Jan. 30, 2013, 8:31 a.m.
Sorry, missing replied... It should be the reply to Eric here.

On 01/30/2013 03:37 PM, Lei Li wrote:
> On 01/29/2013 04:24 AM, Anthony Liguori wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 01/27/2013 11:14 AM, Lei Li wrote:
>>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>>> ---
>>>>   include/qapi/qmp/qerror.h |    3 +++
>>>>   qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
>>>>   qga/qapi-schema.json      |   38 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 71 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>>>> index 6c0a18d..0baf1a4 100644
>>>> --- a/include/qapi/qmp/qerror.h
>>>> +++ b/include/qapi/qmp/qerror.h
>>>> @@ -129,6 +129,9 @@ void assert_no_error(Error *err);
>>>>   #define QERR_FEATURE_DISABLED \
>>>>       ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>>>>   +#define QERR_GET_TIME_FAILED \
>>>> +    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
>>>> +
>>> These days, you shouldn't be defining a new QERR wrapper. 
>>> Instead,...[1]
>>>
>>>>   +static TimeInfo *get_host_time(Error **errp)
>>> This name is unusual...[2]
>>>
>>>> +{
>>>> +    int ret;
>>>> +    qemu_timeval tq;
>>>> +    TimeInfo *time_info;
>>>> +
>>>> +    ret = qemu_gettimeofday(&tq);
>>>> +    if (ret < 0) {
>>>> +        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
>>> [1]...use the right idiom here:
>>>
>>> error_setg_errno(errp, errno, "Failed to get time");
>>>
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    time_info = g_malloc0(sizeof(TimeInfo));
>>>> +    time_info->seconds = tq.tv_sec;
>>>> +    time_info->microseconds = tq.tv_usec;
>>> Is microseconds the right unit to expose through JSON, or are we going
>>> to wish we had exposed nanoseconds down the road (you can still use
>>> qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
>>> time_info->nanoseconds, if we desire the extra granularity).
>>>
>>> You aren't setting time_info->utc_offset.  Is that intentional?
>> Moreover, there's no need to specify microseconds and seconds. A 64-bit
>> value for nanoseconds is sufficient.  A signed value can represent
>> 1678 -> 2262.  If anyone is using qemu-ga in 2262, they'll have to
>> introduce a new command for their quantum emulators :-)
>>
>> Setting time independent of date is a bit silly because time cannot be
>> interpreted correctly without a date.
>>
>> A single value of nanoseconds since the epoch (interpreted as UTC/GMT
>> time) is really the best strategy.  The host shouldn't be involved in
>> guest time zones.  In a Cloud environment, it's pretty normal to have
>> different guests using different time zones.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>> +
>>>> +    return time_info;
>>>> +}
>>>> +
>>>> +struct TimeInfo *qmp_guest_get_time(Error **errp)
>>>> +{
>>>> +    TimeInfo *time_info = get_host_time(errp);
>>> [2]...given that it is only ever called from qmp_guest_get_time.
>>>
>>>> +
>>>> +    if (!time_info) {
>>>> +        return NULL;
>>>> +    }
>>> These three lines can be deleted,
>>>
>>>> +
>>>> +    return time_info;
>>> since this line will do the same thing if you let NULL time_info get
>>> this far.
>>>
>>>> +++ b/qga/qapi-schema.json
>>>> @@ -83,6 +83,44 @@
>>>>   { 'command': 'guest-ping' }
>>>>     ##
>>>> +# @TimeInfo
>>>> +#
>>>> +# Time Information. It is relative to the Epoch of 1970-01-01.
>>>> +#
>>>> +# @seconds: "seconds" time unit.
>>>> +#
>>>> +# @microseconds: "microseconds" time unit.
>>>> +#
>>>> +# @utc-offset: Information about utc offset. Represented as:
>>>> +#              ±[mmmm] based at a minimum on minutes, with
>>> s/based at a minimum on//
>>>
>>> This still doesn't state whether two hours east of UTC is 
>>> represented as
>>> 120 or as 0200.
>>>
> It should be 120.
> Yeah, I should make it clear.
>
> I am thinking if this 'utc_offset' can be made as a string, represented
> like: ±[hh]:[mm], +08:45 (8 hours and 45 minutes) for example.
>
>>>> +#              negative values are west and positive values
>>>> +#              are east of UTC. The bounds of @utc-offset is
>>>> +#              at most 24 hours away from UTC.
>>>> +#
>>>> +# Since: 1.4
>>>> +##
>>>> +{ 'type': 'TimeInfo',
>>>> +  'data': { 'seconds': 'int', 'microseconds': 'int',
>>>> +            'utc-offset': 'int' } }
>>>> +
>>>> +##
>>>> +# @guest-get-time:
>>>> +#
>>>> +# Get the information about host time in UTC and the
>>> s/host/guest/
>>>
>>>> +# UTC offset.
>>>> +#
>>>> +# This command tries to get the host time which is
>>>> +# presumably correct, since we need to be able to
>>>> +# resynchronize guest clock to host's in guest.
>>> This sentence doesn't make sense.  Better would be:
>>>
>>> Get the guest's notion of the current time.
>>>
>>>> +#
>>>> +# Returns: @TimeInfo on success.
>>>> +#
>>>> +# Since 1.4
>>>> +##
>>>> +{ 'command': 'guest-get-time',
>>>> +  'returns': 'TimeInfo' }
>>>> +
>>>> +##
>>>>   # @GuestAgentCommandInfo:
>>>>   #
>>>>   # Information about guest agent commands.
>>>>
>>> -- 
>>> Eric Blake   eblake redhat com    +1-919-301-3266
>>> Libvirt virtualization library http://libvirt.org
>
>
Michael Roth - Feb. 6, 2013, 1:06 a.m.
On Wed, Jan 30, 2013 at 03:37:25PM +0800, Lei Li wrote:
> On 01/29/2013 04:24 AM, Anthony Liguori wrote:
> >Eric Blake <eblake@redhat.com> writes:
> >
> >>On 01/27/2013 11:14 AM, Lei Li wrote:
> >>>Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >>>---
> >>>  include/qapi/qmp/qerror.h |    3 +++
> >>>  qga/commands-posix.c      |   30 ++++++++++++++++++++++++++++++
> >>>  qga/qapi-schema.json      |   38 ++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 71 insertions(+), 0 deletions(-)
> >>>
> >>>diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> >>>index 6c0a18d..0baf1a4 100644
> >>>--- a/include/qapi/qmp/qerror.h
> >>>+++ b/include/qapi/qmp/qerror.h
> >>>@@ -129,6 +129,9 @@ void assert_no_error(Error *err);
> >>>  #define QERR_FEATURE_DISABLED \
> >>>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
> >>>+#define QERR_GET_TIME_FAILED \
> >>>+    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
> >>>+
> >>These days, you shouldn't be defining a new QERR wrapper.  Instead,...[1]
> >>
> >>>+static TimeInfo *get_host_time(Error **errp)
> >>This name is unusual...[2]
> >>
> >>>+{
> >>>+    int ret;
> >>>+    qemu_timeval tq;
> >>>+    TimeInfo *time_info;
> >>>+
> >>>+    ret = qemu_gettimeofday(&tq);
> >>>+    if (ret < 0) {
> >>>+        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
> >>[1]...use the right idiom here:
> >>
> >>error_setg_errno(errp, errno, "Failed to get time");
> >>
> >>>+        return NULL;
> >>>+    }
> >>>+
> >>>+    time_info = g_malloc0(sizeof(TimeInfo));
> >>>+    time_info->seconds = tq.tv_sec;
> >>>+    time_info->microseconds = tq.tv_usec;
> >>Is microseconds the right unit to expose through JSON, or are we going
> >>to wish we had exposed nanoseconds down the road (you can still use
> >>qemu_gettimeofday() and scale tv_usec by 1000 before assigning to
> >>time_info->nanoseconds, if we desire the extra granularity).
> >>
> >>You aren't setting time_info->utc_offset.  Is that intentional?
> >Moreover, there's no need to specify microseconds and seconds.  A 64-bit
> >value for nanoseconds is sufficient.  A signed value can represent
> >1678 -> 2262.  If anyone is using qemu-ga in 2262, they'll have to
> >introduce a new command for their quantum emulators :-)
> >
> >Setting time independent of date is a bit silly because time cannot be
> >interpreted correctly without a date.
> >
> >A single value of nanoseconds since the epoch (interpreted as UTC/GMT
> >time) is really the best strategy.  The host shouldn't be involved in
> >guest time zones.  In a Cloud environment, it's pretty normal to have
> >different guests using different time zones.
> >
> >Regards,
> >
> >Anthony Liguori
> >
> >>>+
> >>>+    return time_info;
> >>>+}
> >>>+
> >>>+struct TimeInfo *qmp_guest_get_time(Error **errp)
> >>>+{
> >>>+    TimeInfo *time_info = get_host_time(errp);
> >>[2]...given that it is only ever called from qmp_guest_get_time.
> >>
> >>>+
> >>>+    if (!time_info) {
> >>>+        return NULL;
> >>>+    }
> >>These three lines can be deleted,
> >>
> >>>+
> >>>+    return time_info;
> >>since this line will do the same thing if you let NULL time_info get
> >>this far.
> >>
> >>>+++ b/qga/qapi-schema.json
> >>>@@ -83,6 +83,44 @@
> >>>  { 'command': 'guest-ping' }
> >>>  ##
> >>>+# @TimeInfo
> >>>+#
> >>>+# Time Information. It is relative to the Epoch of 1970-01-01.
> >>>+#
> >>>+# @seconds: "seconds" time unit.
> >>>+#
> >>>+# @microseconds: "microseconds" time unit.
> >>>+#
> >>>+# @utc-offset: Information about utc offset. Represented as:
> >>>+#              ±[mmmm] based at a minimum on minutes, with
> >>s/based at a minimum on//
> >>
> >>This still doesn't state whether two hours east of UTC is represented as
> >>120 or as 0200.
> >>
> It should be 120.
> Yeah, I should make it clear.
> 
> I am thinking if this 'utc_offset' can be made as a string, represented
> like: ±[hh]:[mm], +08:45 (8 hours and 45 minutes) for example.

I'd prefer we stick with an integer value representing minutes.
QMP/qemu-ga are programmatic interfaces where human-readable string
representations are actually harder to work with for this type of thing.

> 
> >>>+#              negative values are west and positive values
> >>>+#              are east of UTC. The bounds of @utc-offset is
> >>>+#              at most 24 hours away from UTC.
> >>>+#
> >>>+# Since: 1.4
> >>>+##
> >>>+{ 'type': 'TimeInfo',
> >>>+  'data': { 'seconds': 'int', 'microseconds': 'int',
> >>>+            'utc-offset': 'int' } }
> >>>+
> >>>+##
> >>>+# @guest-get-time:
> >>>+#
> >>>+# Get the information about host time in UTC and the
> >>s/host/guest/
> >>
> >>>+# UTC offset.
> >>>+#
> >>>+# This command tries to get the host time which is
> >>>+# presumably correct, since we need to be able to
> >>>+# resynchronize guest clock to host's in guest.
> >>This sentence doesn't make sense.  Better would be:
> >>
> >>Get the guest's notion of the current time.
> >>
> >>>+#
> >>>+# Returns: @TimeInfo on success.
> >>>+#
> >>>+# Since 1.4
> >>>+##
> >>>+{ 'command': 'guest-get-time',
> >>>+  'returns': 'TimeInfo' }
> >>>+
> >>>+##
> >>>  # @GuestAgentCommandInfo:
> >>>  #
> >>>  # Information about guest agent commands.
> >>>
> >>-- 
> >>Eric Blake   eblake redhat com    +1-919-301-3266
> >>Libvirt virtualization library http://libvirt.org
> 
> 
> -- 
> Lei
>

Patch

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6c0a18d..0baf1a4 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -129,6 +129,9 @@  void assert_no_error(Error *err);
 #define QERR_FEATURE_DISABLED \
     ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
 
+#define QERR_GET_TIME_FAILED \
+    ERROR_CLASS_GENERIC_ERROR, "Failed to get time"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
 
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ad73f3..2fef2b6 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -119,6 +119,36 @@  void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     /* succeded */
 }
 
+static TimeInfo *get_host_time(Error **errp)
+{
+    int ret;
+    qemu_timeval tq;
+    TimeInfo *time_info;
+
+    ret = qemu_gettimeofday(&tq);
+    if (ret < 0) {
+        error_set_errno(errp, errno, QERR_GET_TIME_FAILED);
+        return NULL;
+    }
+
+    time_info = g_malloc0(sizeof(TimeInfo));
+    time_info->seconds = tq.tv_sec;
+    time_info->microseconds = tq.tv_usec;
+
+    return time_info;
+}
+
+struct TimeInfo *qmp_guest_get_time(Error **errp)
+{
+    TimeInfo *time_info = get_host_time(errp);
+
+    if (!time_info) {
+        return NULL;
+    }
+
+    return time_info;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index d91d903..d067fa5 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -83,6 +83,44 @@ 
 { 'command': 'guest-ping' }
 
 ##
+# @TimeInfo
+#
+# Time Information. It is relative to the Epoch of 1970-01-01.
+#
+# @seconds: "seconds" time unit.
+#
+# @microseconds: "microseconds" time unit.
+#
+# @utc-offset: Information about utc offset. Represented as:
+#              ±[mmmm] based at a minimum on minutes, with
+#              negative values are west and positive values
+#              are east of UTC. The bounds of @utc-offset is
+#              at most 24 hours away from UTC.
+#
+# Since: 1.4
+##
+{ 'type': 'TimeInfo',
+  'data': { 'seconds': 'int', 'microseconds': 'int',
+            'utc-offset': 'int' } }
+
+##
+# @guest-get-time:
+#
+# Get the information about host time in UTC and the
+# UTC offset.
+#
+# This command tries to get the host time which is
+# presumably correct, since we need to be able to
+# resynchronize guest clock to host's in guest.
+#
+# Returns: @TimeInfo on success.
+#
+# Since 1.4
+##
+{ 'command': 'guest-get-time',
+  'returns': 'TimeInfo' }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.