| Message ID | 1359310460-23564-2-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/27/2013 11:14 AM, Lei Li wrote: > Signed-off-by: Lei Li <lilei@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? > + > + 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> writes: > On 01/27/2013 11:14 AM, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@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. > >> +# 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
On 01/29/2013 02:29 AM, Eric Blake wrote: > On 01/27/2013 11:14 AM, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@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] Okay, I think there might be misunderstanding here. The current implementation of this 'guest-get-time' command is trying to get the* *_host_ time, since the guest time might be not correct. This qemu_gettimeofday should return the host time, or am I wrong? >> +{ >> + 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? Yes, since 'guest-get-time' is supposed to get host time in UTC, the utc_offset is zero. When guest want to set different time zone it can set the utc_offset through 'guest-set-time'. >> + >> + 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. >>
On 01/29/2013 04:24 AM, Anthony Liguori wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 01/27/2013 11:14 AM, Lei Li wrote: >>> Signed-off-by: Lei Li <lilei@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. >>> +# 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
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 <eblake@redhat.com> writes: >> >>> On 01/27/2013 11:14 AM, Lei Li wrote: >>>> Signed-off-by: Lei Li <lilei@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. > >>>> +# 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 > >
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 <eblake@redhat.com> writes: > > > >>On 01/27/2013 11:14 AM, Lei Li wrote: > >>>Signed-off-by: Lei Li <lilei@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 >
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" + #define QERR_INVALID_BLOCK_FORMAT \ ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'" diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 0ad73f3..2fef2b6 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -119,6 +119,36 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) /* succeded */ } +static TimeInfo *get_host_time(Error **errp) +{ + int ret; + qemu_timeval tq; + TimeInfo *time_info; + + ret = qemu_gettimeofday(&tq); + if (ret < 0) { + error_set_errno(errp, errno, QERR_GET_TIME_FAILED); + return NULL; + } + + time_info = g_malloc0(sizeof(TimeInfo)); + time_info->seconds = tq.tv_sec; + time_info->microseconds = tq.tv_usec; + + return time_info; +} + +struct TimeInfo *qmp_guest_get_time(Error **errp) +{ + TimeInfo *time_info = get_host_time(errp); + + if (!time_info) { + return NULL; + } + + return time_info; +} + typedef struct GuestFileHandle { uint64_t id; FILE *fh; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index d91d903..d067fa5 100644 --- a/qga/qapi-schema.json +++ 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 +# 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 +# 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. +# +# Returns: @TimeInfo on success. +# +# Since 1.4 +## +{ 'command': 'guest-get-time', + 'returns': 'TimeInfo' } + +## # @GuestAgentCommandInfo: # # Information about guest agent commands.
Signed-off-by: Lei Li <lilei@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(-)