[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: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command |
Date: |
Mon, 28 Jan 2013 11:29:10 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 |
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?
> +
> + 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
signature.asc
Description: OpenPGP digital signature