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 <ebl...@redhat.com> writes: > > > >>On 01/27/2013 11:14 AM, Lei Li wrote: > >>>Signed-off-by: Lei Li <li...@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 >