| Message ID | 1357466820-12860-4-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/06/2013 03:07 AM, Lei Li wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qga/commands-posix.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ > qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ > 2 files changed, 89 insertions(+), 0 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 190199d..7fff49a 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp) > return host_time; > } > > +void qmp_guest_set_time(bool has_seconds, int64_t seconds, > + bool has_microseconds, int64_t microseconds, > + bool has_utc_offset, int64_t utc_offset, Error **errp) > +{ > + int ret; > + int status; > + pid_t pid, rpid; > + struct timeval tv; > + HostTimeInfo *host_time; > + > + if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) { Is it really qemu style to parenthesize this much? > + host_time = get_host_time(); > + if (!host_time) { > + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time"); > + return; > + } > + tv.tv_sec = host_time->seconds; > + tv.tv_usec = host_time->microseconds; > + } else if (has_seconds && has_microseconds && has_utc_offset) { > + tv.tv_sec = (time_t) seconds + utc_offset; You need to worry about overflow on hosts where time_t is 32-bits but the user passed time using 64-bits (such as past the year 2038). Likewise, it might be worth bounds-checking utc-offset to be at most 12 hours away from UTC (or is there a better bounds?). > + tv.tv_usec = (time_t) microseconds; Likewise, you should range-validate that microseconds does not overflow 1000000 (or, if you take my suggestion about using nanoseconds, since struct timespec is a bit more expressive, then bound things by 1000000000, and properly round when converting to lower resolution interfaces such as settimeofday()). > + } else { > + error_set(errp, QERR_INVALID_PARAMETER, "parameter missing"); That's a bit harsh. I'm thinking it might be nicer to support: all three missing - grab time from the host at least seconds present - populate any missing subseconds or utc_offset as 0 seconds missing, but other fields present - error making this look more like: if (!has_seconds) { if (has_subseconds || has_utc_offset) { error_set(); } else { use get_host_time(); } } else { tv.tv_sec = seconds + (has_utc_offset ? utc_offset : 0); ... } > +++ b/qga/qapi-schema.json > @@ -117,6 +117,38 @@ > 'returns': 'HostTimeInfo' } > > ## > +# @guest-set-time: > +# > +# Set guest time. If none arguments were given, will set s/none/no/ > +# host time to guest. > +# > +# Right now, when a guest is paused or migrated to a file > +# then loaded from that file, the guest OS has no idea that > +# there was a big gap in the time. Depending on how long > +# the gap was, NTP might not be able to resynchronize the > +# guest. > +# > +# This command tries to set guest time based on the information > +# from host or an absolute value given by management app, and > +# set the Hardware Clock to the current System Time. This > +# will make it easier for a guest to resynchronize without > +# waiting for NTP. > +# > +# @seconds: #optional "seconds" time. > +# > +# @microseconds: #optional "microseconds" time. > +# > +# @utc-offset: #optional utc offset. If you like my above suggestions, this might be worth documenting that @microseconds (or @nanoseconds) must not be provided unless @seconds is present, and so on. Same questions as in patch 1/3 - you need to document what @seconds is relative to (presumably the Epoch of 1970-01-01), and what format utc-offset takes. Based on this patch, it looks like you are using utc-offset as the number of seconds difference, so one hour is represented as 3600.
On Sun, 6 Jan 2013 18:07:00 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qga/commands-posix.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ > qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ > 2 files changed, 89 insertions(+), 0 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 190199d..7fff49a 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp) > return host_time; > } > > +void qmp_guest_set_time(bool has_seconds, int64_t seconds, > + bool has_microseconds, int64_t microseconds, > + bool has_utc_offset, int64_t utc_offset, Error **errp) > +{ > + int ret; > + int status; > + pid_t pid, rpid; > + struct timeval tv; > + HostTimeInfo *host_time; > + > + if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) { > + host_time = get_host_time(); > + if (!host_time) { > + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time"); If you change get_host_time() to take an Error * argument, you can drop this. > + return; > + } > + tv.tv_sec = host_time->seconds; > + tv.tv_usec = host_time->microseconds; > + } else if (has_seconds && has_microseconds && has_utc_offset) { > + tv.tv_sec = (time_t) seconds + utc_offset; > + tv.tv_usec = (time_t) microseconds; > + } else { > + error_set(errp, QERR_INVALID_PARAMETER, "parameter missing"); Please, use error_setg() instead. > + return; > + } > + > + ret = settimeofday(&tv, NULL); > + if (ret < 0) { > + error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno)); Please, use error_setg_errno(). > + return; > + } > + > + /* Set the Hardware Clock to the current System Time. */ > + pid = fork(); > + if (pid == 0) { > + setsid(); > + reopen_fd_to_null(0); > + reopen_fd_to_null(1); > + reopen_fd_to_null(2); > + > + execle("/sbin/hwclock", "hwclock", "-w", NULL, environ); Honest question: is this really necessary? Can't we do whatever hwclock does? > + _exit(EXIT_FAILURE); > + } else if (pid < 0) { > + goto exit_err; > + } > + > + do { > + rpid = waitpid(pid, &status, 0); > + } while (rpid == -1 && errno == EINTR); > + if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) { > + return; > + } > + > +exit_err: > + error_set(errp, QERR_UNDEFINED_ERROR); > +} > + > typedef struct GuestFileHandle { > uint64_t id; > FILE *fh; > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 4a8b93c..4649b55 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -117,6 +117,38 @@ > 'returns': 'HostTimeInfo' } > > ## > +# @guest-set-time: > +# > +# Set guest time. If none arguments were given, will set > +# host time to guest. > +# > +# Right now, when a guest is paused or migrated to a file > +# then loaded from that file, the guest OS has no idea that > +# there was a big gap in the time. Depending on how long > +# the gap was, NTP might not be able to resynchronize the > +# guest. > +# > +# This command tries to set guest time based on the information > +# from host or an absolute value given by management app, and > +# set the Hardware Clock to the current System Time. This > +# will make it easier for a guest to resynchronize without > +# waiting for NTP. > +# > +# @seconds: #optional "seconds" time. > +# > +# @microseconds: #optional "microseconds" time. > +# > +# @utc-offset: #optional utc offset. > +# > +# Returns: Nothing on success. > +# > +# Since: 1.4 > +## > +{ 'command': 'guest-set-time', > + 'data': { '*seconds': 'int', '*microseconds': 'int', > + '*utc-offset': 'int' } } > + > +## > # @GuestAgentCommandInfo: > # > # Information about guest agent commands.
On 01/08/2013 06:26 AM, Eric Blake wrote: > On 01/06/2013 03:07 AM, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qga/commands-posix.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ >> 2 files changed, 89 insertions(+), 0 deletions(-) >> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index 190199d..7fff49a 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp) >> return host_time; >> } >> >> +void qmp_guest_set_time(bool has_seconds, int64_t seconds, >> + bool has_microseconds, int64_t microseconds, >> + bool has_utc_offset, int64_t utc_offset, Error **errp) >> +{ >> + int ret; >> + int status; >> + pid_t pid, rpid; >> + struct timeval tv; >> + HostTimeInfo *host_time; >> + >> + if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) { > Is it really qemu style to parenthesize this much? > >> + host_time = get_host_time(); >> + if (!host_time) { >> + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time"); >> + return; >> + } >> + tv.tv_sec = host_time->seconds; >> + tv.tv_usec = host_time->microseconds; >> + } else if (has_seconds && has_microseconds && has_utc_offset) { >> + tv.tv_sec = (time_t) seconds + utc_offset; > You need to worry about overflow on hosts where time_t is 32-bits but > the user passed time using 64-bits (such as past the year 2038). > Likewise, it might be worth bounds-checking utc-offset to be at most 12 > hours away from UTC (or is there a better bounds?). > >> + tv.tv_usec = (time_t) microseconds; > Likewise, you should range-validate that microseconds does not overflow > 1000000 (or, if you take my suggestion about using nanoseconds, since > struct timespec is a bit more expressive, then bound things by > 1000000000, and properly round when converting to lower resolution > interfaces such as settimeofday()). > >> + } else { >> + error_set(errp, QERR_INVALID_PARAMETER, "parameter missing"); > That's a bit harsh. I'm thinking it might be nicer to support: > > all three missing - grab time from the host > at least seconds present - populate any missing subseconds or utc_offset > as 0 > seconds missing, but other fields present - error > > making this look more like: > > if (!has_seconds) { > if (has_subseconds || has_utc_offset) { > error_set(); > } else { > use get_host_time(); > } > } else { > tv.tv_sec = seconds + (has_utc_offset ? utc_offset : 0); > ... > } Good suggestions! Yes, I know this is harsh. I will improve it in next version, as well as the document. >> +++ b/qga/qapi-schema.json >> @@ -117,6 +117,38 @@ >> 'returns': 'HostTimeInfo' } >> >> ## >> +# @guest-set-time: >> +# >> +# Set guest time. If none arguments were given, will set > s/none/no/ > >> +# host time to guest. >> +# >> +# Right now, when a guest is paused or migrated to a file >> +# then loaded from that file, the guest OS has no idea that >> +# there was a big gap in the time. Depending on how long >> +# the gap was, NTP might not be able to resynchronize the >> +# guest. >> +# >> +# This command tries to set guest time based on the information >> +# from host or an absolute value given by management app, and >> +# set the Hardware Clock to the current System Time. This >> +# will make it easier for a guest to resynchronize without >> +# waiting for NTP. >> +# >> +# @seconds: #optional "seconds" time. >> +# >> +# @microseconds: #optional "microseconds" time. >> +# >> +# @utc-offset: #optional utc offset. > If you like my above suggestions, this might be worth documenting that > @microseconds (or @nanoseconds) must not be provided unless @seconds is > present, and so on. > > > Same questions as in patch 1/3 - you need to document what @seconds is > relative to (presumably the Epoch of 1970-01-01), and what format > utc-offset takes. Based on this patch, it looks like you are using > utc-offset as the number of seconds difference, so one hour is > represented as 3600. Sure. About the utc-offset format, I have replied to the previous patch. It would be one-hour offset, and it's my mistake here...:( >
On 01/09/2013 09:40 PM, Luiz Capitulino wrote: > On Sun, 6 Jan 2013 18:07:00 +0800 > Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qga/commands-posix.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ >> 2 files changed, 89 insertions(+), 0 deletions(-) >> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index 190199d..7fff49a 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp) >> return host_time; >> } >> >> +void qmp_guest_set_time(bool has_seconds, int64_t seconds, >> + bool has_microseconds, int64_t microseconds, >> + bool has_utc_offset, int64_t utc_offset, Error **errp) >> +{ >> + int ret; >> + int status; >> + pid_t pid, rpid; >> + struct timeval tv; >> + HostTimeInfo *host_time; >> + >> + if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) { >> + host_time = get_host_time(); >> + if (!host_time) { >> + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time"); > If you change get_host_time() to take an Error * argument, you can drop this. ok. >> + return; >> + } >> + tv.tv_sec = host_time->seconds; >> + tv.tv_usec = host_time->microseconds; >> + } else if (has_seconds && has_microseconds && has_utc_offset) { >> + tv.tv_sec = (time_t) seconds + utc_offset; >> + tv.tv_usec = (time_t) microseconds; >> + } else { >> + error_set(errp, QERR_INVALID_PARAMETER, "parameter missing"); > Please, use error_setg() instead. Sure. >> + return; >> + } >> + >> + ret = settimeofday(&tv, NULL); >> + if (ret < 0) { >> + error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno)); > Please, use error_setg_errno(). > Yes. >> + return; >> + } >> + >> + /* Set the Hardware Clock to the current System Time. */ >> + pid = fork(); >> + if (pid == 0) { >> + setsid(); >> + reopen_fd_to_null(0); >> + reopen_fd_to_null(1); >> + reopen_fd_to_null(2); >> + >> + execle("/sbin/hwclock", "hwclock", "-w", NULL, environ); > Honest question: is this really necessary? Can't we do whatever hwclock does? > I have thought about implementing this ourselves, and I did take a look at the source code of hwclock. But looks like the implement ofthe synchronization for hardware clock and system clock is a little complicated, so I am not sure if it's worth to have a try here. >> + _exit(EXIT_FAILURE); >> + } else if (pid < 0) { >> + goto exit_err; >> + } >> + >> + do { >> + rpid = waitpid(pid, &status, 0); >> + } while (rpid == -1 && errno == EINTR); >> + if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) { >> + return; >> + } >> + >> +exit_err: >> + error_set(errp, QERR_UNDEFINED_ERROR); >> +} >> + >> typedef struct GuestFileHandle { >> uint64_t id; >> FILE *fh; >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >> index 4a8b93c..4649b55 100644 >> --- a/qga/qapi-schema.json >> +++ b/qga/qapi-schema.json >> @@ -117,6 +117,38 @@ >> 'returns': 'HostTimeInfo' } >> >> ## >> +# @guest-set-time: >> +# >> +# Set guest time. If none arguments were given, will set >> +# host time to guest. >> +# >> +# Right now, when a guest is paused or migrated to a file >> +# then loaded from that file, the guest OS has no idea that >> +# there was a big gap in the time. Depending on how long >> +# the gap was, NTP might not be able to resynchronize the >> +# guest. >> +# >> +# This command tries to set guest time based on the information >> +# from host or an absolute value given by management app, and >> +# set the Hardware Clock to the current System Time. This >> +# will make it easier for a guest to resynchronize without >> +# waiting for NTP. >> +# >> +# @seconds: #optional "seconds" time. >> +# >> +# @microseconds: #optional "microseconds" time. >> +# >> +# @utc-offset: #optional utc offset. >> +# >> +# Returns: Nothing on success. >> +# >> +# Since: 1.4 >> +## >> +{ 'command': 'guest-set-time', >> + 'data': { '*seconds': 'int', '*microseconds': 'int', >> + '*utc-offset': 'int' } } >> + >> +## >> # @GuestAgentCommandInfo: >> # >> # Information about guest agent commands. >
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 190199d..7fff49a 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -121,6 +121,63 @@ struct HostTimeInfo *qmp_guest_get_time(Error **errp) return host_time; } +void qmp_guest_set_time(bool has_seconds, int64_t seconds, + bool has_microseconds, int64_t microseconds, + bool has_utc_offset, int64_t utc_offset, Error **errp) +{ + int ret; + int status; + pid_t pid, rpid; + struct timeval tv; + HostTimeInfo *host_time; + + if ((!has_seconds) && (!has_microseconds) && (!has_utc_offset)) { + host_time = get_host_time(); + if (!host_time) { + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set guest time"); + return; + } + tv.tv_sec = host_time->seconds; + tv.tv_usec = host_time->microseconds; + } else if (has_seconds && has_microseconds && has_utc_offset) { + tv.tv_sec = (time_t) seconds + utc_offset; + tv.tv_usec = (time_t) microseconds; + } else { + error_set(errp, QERR_INVALID_PARAMETER, "parameter missing"); + return; + } + + ret = settimeofday(&tv, NULL); + if (ret < 0) { + error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno)); + return; + } + + /* Set the Hardware Clock to the current System Time. */ + pid = fork(); + if (pid == 0) { + setsid(); + reopen_fd_to_null(0); + reopen_fd_to_null(1); + reopen_fd_to_null(2); + + execle("/sbin/hwclock", "hwclock", "-w", NULL, environ); + _exit(EXIT_FAILURE); + } else if (pid < 0) { + goto exit_err; + } + + do { + rpid = waitpid(pid, &status, 0); + } while (rpid == -1 && errno == EINTR); + if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) { + return; + } + +exit_err: + error_set(errp, QERR_UNDEFINED_ERROR); +} + typedef struct GuestFileHandle { uint64_t id; FILE *fh; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 4a8b93c..4649b55 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -117,6 +117,38 @@ 'returns': 'HostTimeInfo' } ## +# @guest-set-time: +# +# Set guest time. If none arguments were given, will set +# host time to guest. +# +# Right now, when a guest is paused or migrated to a file +# then loaded from that file, the guest OS has no idea that +# there was a big gap in the time. Depending on how long +# the gap was, NTP might not be able to resynchronize the +# guest. +# +# This command tries to set guest time based on the information +# from host or an absolute value given by management app, and +# set the Hardware Clock to the current System Time. This +# will make it easier for a guest to resynchronize without +# waiting for NTP. +# +# @seconds: #optional "seconds" time. +# +# @microseconds: #optional "microseconds" time. +# +# @utc-offset: #optional utc offset. +# +# Returns: Nothing on success. +# +# Since: 1.4 +## +{ 'command': 'guest-set-time', + '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 | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 0 deletions(-)