[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: |
mdroth |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command |
Date: |
Tue, 5 Feb 2013 19:06:56 -0600 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <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.
> >>
> 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
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command,
mdroth <=