[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command |
Date: |
Mon, 28 Jan 2013 14:24:29 -0600 |
User-agent: |
Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Eric Blake <address@hidden> writes:
> On 01/27/2013 11:14 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <address@hidden>
>> ---
>> 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