qemu-devel
[Top][All Lists]
Advanced

[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: Lei Li
Subject: Re: [Qemu-devel] [PATCH 1/2] qga: add guest-get-time command
Date: Wed, 30 Jan 2013 16:31:12 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0

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 <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.

+#              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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]