| Message ID | 1357466820-12860-2-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/06/2013 03:06 AM, Lei Li wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qga/commands-posix.c | 18 ++++++++++++++++++ > qga/qapi-schema.json | 17 +++++++++++++++++ > 2 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index a657201..26b0fa0 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -91,6 +91,24 @@ exit_err: > error_set(err, QERR_UNDEFINED_ERROR); > } > > +static HostTimeInfo *get_host_time(void) > +{ > + host_time = g_malloc0(sizeof(HostTimeInfo)); > + host_time->seconds = tq.tv_sec; > + host_time->microseconds = tq.tv_usec; Why usec? struct timespec with nanoseconds might be a nicer unit, even if for the initial implementation, you use qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime library for full ns resolution. If nothing else, the lesson that ought to be learned from the proliferation of time types is that any time you don't report lots of precision, someone comes along later on having to add yet another interface adding more precision. > +++ b/qga/qapi-schema.json > @@ -83,6 +83,23 @@ > { 'command': 'guest-ping' } > > ## > +# @HostTimeInfo > +# > +# Information about host time. > +# > +# @seconds: "seconds" time from the host. Document that this is relative to the Epoch of 1970-01-01 (no matter what the host uses for its internal reference point). > +# > +# @microseconds: "microseconds" time from the host. Again, nanoseconds (struct timespec) might be nicer. > +# > +# @utc-offset: information about utc offset. In what format? Minutes away from UTC, a 4-digit decimal value, or something else (that is, is a one-hour offset represented as 60 or 100)? Are negative values east or west of UTC? > +# > +# Since: 1.4 > +## > +{ 'type': 'HostTimeInfo', > + 'data': { 'seconds': 'int', 'microseconds': 'int', > + 'utc-offset': 'int' } } Indentation seems inconsistent. Ah, here you made them mandatory - only your cover letter implied that they were optional.
On Sun, 6 Jan 2013 18:06:58 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qga/commands-posix.c | 18 ++++++++++++++++++ > qga/qapi-schema.json | 17 +++++++++++++++++ > 2 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index a657201..26b0fa0 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -91,6 +91,24 @@ exit_err: > error_set(err, QERR_UNDEFINED_ERROR); > } > > +static HostTimeInfo *get_host_time(void) > +{ Does this build? Because no one is using this function. > + int err; > + qemu_timeval tq; > + HostTimeInfo *host_time; > + > + err = qemu_gettimeofday(&tq); > + if (err < 0) { I'd recommend taking an Error * argument and setting it with error_set_errno(). > + return NULL; > + } > + > + host_time = g_malloc0(sizeof(HostTimeInfo)); > + host_time->seconds = tq.tv_sec; > + host_time->microseconds = tq.tv_usec; > + > + return host_time; > +} > + > typedef struct GuestFileHandle { > uint64_t id; > FILE *fh; > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index ed0eb69..7793aff 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -83,6 +83,23 @@ > { 'command': 'guest-ping' } > > ## > +# @HostTimeInfo I'm a bit confused, why do you call it HostTimeInfo if this runs in the guest? > +# > +# Information about host time. > +# > +# @seconds: "seconds" time from the host. > +# > +# @microseconds: "microseconds" time from the host. > +# > +# @utc-offset: information about utc offset. > +# > +# Since: 1.4 > +## > +{ 'type': 'HostTimeInfo', > + 'data': { 'seconds': 'int', 'microseconds': 'int', > + 'utc-offset': 'int' } } > + > +## > # @GuestAgentCommandInfo: > # > # Information about guest agent commands.
On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qga/commands-posix.c | 18 ++++++++++++++++++ > qga/qapi-schema.json | 17 +++++++++++++++++ > 2 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index a657201..26b0fa0 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -91,6 +91,24 @@ exit_err: > error_set(err, QERR_UNDEFINED_ERROR); > } > > +static HostTimeInfo *get_host_time(void) > +{ Should squash this into patch #2, since it doesn't get used till then. Otherwise we'll break build bisection when compiling with -Wunused/-Wall && -Werror, which is usually the default. > + int err; > + qemu_timeval tq; > + HostTimeInfo *host_time; > + > + err = qemu_gettimeofday(&tq); > + if (err < 0) { > + return NULL; > + } > + > + host_time = g_malloc0(sizeof(HostTimeInfo)); > + host_time->seconds = tq.tv_sec; > + host_time->microseconds = tq.tv_usec; > + > + return host_time; > +} > + > typedef struct GuestFileHandle { > uint64_t id; > FILE *fh; > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index ed0eb69..7793aff 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -83,6 +83,23 @@ > { 'command': 'guest-ping' } > > ## > +# @HostTimeInfo > +# > +# Information about host time. > +# > +# @seconds: "seconds" time from the host. > +# > +# @microseconds: "microseconds" time from the host. > +# > +# @utc-offset: information about utc offset. > +# > +# Since: 1.4 > +## > +{ 'type': 'HostTimeInfo', > + 'data': { 'seconds': 'int', 'microseconds': 'int', > + 'utc-offset': 'int' } } > + > +## > # @GuestAgentCommandInfo: > # > # Information about guest agent commands. > -- > 1.7.7.6 >
On 01/08/2013 05:52 AM, Eric Blake wrote: > On 01/06/2013 03:06 AM, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qga/commands-posix.c | 18 ++++++++++++++++++ >> qga/qapi-schema.json | 17 +++++++++++++++++ >> 2 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index a657201..26b0fa0 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -91,6 +91,24 @@ exit_err: >> error_set(err, QERR_UNDEFINED_ERROR); >> } >> >> +static HostTimeInfo *get_host_time(void) >> +{ >> + host_time = g_malloc0(sizeof(HostTimeInfo)); >> + host_time->seconds = tq.tv_sec; >> + host_time->microseconds = tq.tv_usec; > Why usec? struct timespec with nanoseconds might be a nicer unit, even > if for the initial implementation, you use > qemu_gettimeofday().tv_usec*1000 rather than dragging in a realtime > library for full ns resolution. If nothing else, the lesson that ought > to be learned from the proliferation of time types is that any time you > don't report lots of precision, someone comes along later on having to > add yet another interface adding more precision. ok, I will have a try to change it to a ns resolution. > >> +++ b/qga/qapi-schema.json >> @@ -83,6 +83,23 @@ >> { 'command': 'guest-ping' } >> >> ## >> +# @HostTimeInfo >> +# >> +# Information about host time. >> +# >> +# @seconds: "seconds" time from the host. > Document that this is relative to the Epoch of 1970-01-01 (no matter > what the host uses for its internal reference point). Sure. > >> +# >> +# @microseconds: "microseconds" time from the host. > Again, nanoseconds (struct timespec) might be nicer. > >> +# >> +# @utc-offset: information about utc offset. > In what format? Minutes away from UTC, a 4-digit decimal value, or > something else (that is, is a one-hour offset represented as 60 or 100)? > Are negative values east or west of UTC? For this version, it's a one-hour offset represented as:±[hh]. Negative values are west, andpositive values are east of UTC. >> +# >> +# Since: 1.4 >> +## >> +{ 'type': 'HostTimeInfo', >> + 'data': { 'seconds': 'int', 'microseconds': 'int', >> + 'utc-offset': 'int' } } > Indentation seems inconsistent. > > Ah, here you made them mandatory - only your cover letter implied that > they were optional. Sorry for the inconsistent indentation. No, this is just the definition of the time structure. The optional arguments mentioned in the cover letter are for command "guest-set-time". :)
On 01/09/2013 09:32 PM, Luiz Capitulino wrote: > On Sun, 6 Jan 2013 18:06:58 +0800 > Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qga/commands-posix.c | 18 ++++++++++++++++++ >> qga/qapi-schema.json | 17 +++++++++++++++++ >> 2 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index a657201..26b0fa0 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -91,6 +91,24 @@ exit_err: >> error_set(err, QERR_UNDEFINED_ERROR); >> } >> >> +static HostTimeInfo *get_host_time(void) >> +{ > Does this build? Because no one is using this function. Yes, this should be squashed into patch #2 as Mike also pointed out that. >> + int err; >> + qemu_timeval tq; >> + HostTimeInfo *host_time; >> + >> + err = qemu_gettimeofday(&tq); >> + if (err < 0) { > I'd recommend taking an Error * argument and setting it with > error_set_errno(). ok. > >> + return NULL; >> + } >> + >> + host_time = g_malloc0(sizeof(HostTimeInfo)); >> + host_time->seconds = tq.tv_sec; >> + host_time->microseconds = tq.tv_usec; >> + >> + return host_time; >> +} >> + >> typedef struct GuestFileHandle { >> uint64_t id; >> FILE *fh; >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >> index ed0eb69..7793aff 100644 >> --- a/qga/qapi-schema.json >> +++ b/qga/qapi-schema.json >> @@ -83,6 +83,23 @@ >> { 'command': 'guest-ping' } >> >> ## >> +# @HostTimeInfo > I'm a bit confused, why do you call it HostTimeInfo if this runs > in the guest? I call it HostTimeInfo because it contains the host time information. But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo' might be better? >> +# >> +# Information about host time. >> +# >> +# @seconds: "seconds" time from the host. >> +# >> +# @microseconds: "microseconds" time from the host. >> +# >> +# @utc-offset: information about utc offset. >> +# >> +# Since: 1.4 >> +## >> +{ 'type': 'HostTimeInfo', >> + 'data': { 'seconds': 'int', 'microseconds': 'int', >> + 'utc-offset': 'int' } } >> + >> +## >> # @GuestAgentCommandInfo: >> # >> # Information about guest agent commands.
On 01/09/2013 11:36 PM, mdroth wrote: > On Sun, Jan 06, 2013 at 06:06:58PM +0800, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qga/commands-posix.c | 18 ++++++++++++++++++ >> qga/qapi-schema.json | 17 +++++++++++++++++ >> 2 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index a657201..26b0fa0 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -91,6 +91,24 @@ exit_err: >> error_set(err, QERR_UNDEFINED_ERROR); >> } >> >> +static HostTimeInfo *get_host_time(void) >> +{ > Should squash this into patch #2, since it doesn't get used > till then. Otherwise we'll break build bisection when compiling > with -Wunused/-Wall && -Werror, which is usually the default. ok, got it. Thanks! >> + int err; >> + qemu_timeval tq; >> + HostTimeInfo *host_time; >> + >> + err = qemu_gettimeofday(&tq); >> + if (err < 0) { >> + return NULL; >> + } >> + >> + host_time = g_malloc0(sizeof(HostTimeInfo)); >> + host_time->seconds = tq.tv_sec; >> + host_time->microseconds = tq.tv_usec; >> + >> + return host_time; >> +} >> + >> typedef struct GuestFileHandle { >> uint64_t id; >> FILE *fh; >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >> index ed0eb69..7793aff 100644 >> --- a/qga/qapi-schema.json >> +++ b/qga/qapi-schema.json >> @@ -83,6 +83,23 @@ >> { 'command': 'guest-ping' } >> >> ## >> +# @HostTimeInfo >> +# >> +# Information about host time. >> +# >> +# @seconds: "seconds" time from the host. >> +# >> +# @microseconds: "microseconds" time from the host. >> +# >> +# @utc-offset: information about utc offset. >> +# >> +# Since: 1.4 >> +## >> +{ 'type': 'HostTimeInfo', >> + 'data': { 'seconds': 'int', 'microseconds': 'int', >> + 'utc-offset': 'int' } } >> + >> +## >> # @GuestAgentCommandInfo: >> # >> # Information about guest agent commands. >> -- >> 1.7.7.6 >>
On Fri, 11 Jan 2013 15:19:35 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> ## > >> +# @HostTimeInfo > > I'm a bit confused, why do you call it HostTimeInfo if this runs > > in the guest? > > I call it HostTimeInfo because it contains the host time information. qemu-ga runs in the guest, so it's actually the guest time. > But seems that all of you don't like this 'HostTimeInfo', 'TimeInfo' > might be better? Yes, looks better for me.
On 01/11/2013 12:18 AM, Lei Li wrote: > > For this version, it's a one-hour offset represented as:±[hh]. > Negative values are west, andpositive values are east of UTC. Won't work. There are timezones with a half-hour offset. You need to express offset at least in terms of minutes, not just hours.
On 01/11/2013 11:37 PM, Eric Blake wrote: > On 01/11/2013 12:18 AM, Lei Li wrote: >> For this version, it's a one-hour offset represented as:±[hh]. >> Negative values are west, andpositive values are east of UTC. > Won't work. There are timezones with a half-hour offset. You need to > express offset at least in terms of minutes, not just hours. > Yes, I have thought about this. For the timezones with a half-hour offset, it can just represented as +0.5 for example.
On 01/13/2013 08:17 PM, Lei Li wrote: > On 01/11/2013 11:37 PM, Eric Blake wrote: >> On 01/11/2013 12:18 AM, Lei Li wrote: >>> For this version, it's a one-hour offset represented as:±[hh]. >>> Negative values are west, andpositive values are east of UTC. >> Won't work. There are timezones with a half-hour offset. You need to >> express offset at least in terms of minutes, not just hours. >> > Yes, I have thought about this. For the timezones with a half-hour offset, > it can just represented as +0.5 for example. Floating point parsing is a pain compared to integer parsing. Remember, there is no way in POSIX to print a locale-independent floating point number, which forces programs to jump through quite a few hoops to guarantee that they are using '.' for the radix character in JSON output while still honoring the user's locale. Please, use something based at a minimum on minutes (seconds is also possible), not hours,
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index a657201..26b0fa0 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -91,6 +91,24 @@ exit_err: error_set(err, QERR_UNDEFINED_ERROR); } +static HostTimeInfo *get_host_time(void) +{ + int err; + qemu_timeval tq; + HostTimeInfo *host_time; + + err = qemu_gettimeofday(&tq); + if (err < 0) { + return NULL; + } + + host_time = g_malloc0(sizeof(HostTimeInfo)); + host_time->seconds = tq.tv_sec; + host_time->microseconds = tq.tv_usec; + + return host_time; +} + typedef struct GuestFileHandle { uint64_t id; FILE *fh; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index ed0eb69..7793aff 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -83,6 +83,23 @@ { 'command': 'guest-ping' } ## +# @HostTimeInfo +# +# Information about host time. +# +# @seconds: "seconds" time from the host. +# +# @microseconds: "microseconds" time from the host. +# +# @utc-offset: information about utc offset. +# +# Since: 1.4 +## +{ 'type': 'HostTimeInfo', + 'data': { 'seconds': 'int', 'microseconds': 'int', + 'utc-offset': 'int' } } + +## # @GuestAgentCommandInfo: # # Information about guest agent commands.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- qga/commands-posix.c | 18 ++++++++++++++++++ qga/qapi-schema.json | 17 +++++++++++++++++ 2 files changed, 35 insertions(+), 0 deletions(-)