Patchwork [3/3] qga: add guest-set-time command

login
register
mail settings
Submitter Lei Li
Date Jan. 6, 2013, 10:07 a.m.
Message ID <1357466820-12860-4-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/209733/
State New
Headers show

Comments

Lei Li - Jan. 6, 2013, 10:07 a.m.
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(-)
Eric Blake - Jan. 7, 2013, 10:26 p.m.
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.
Luiz Capitulino - Jan. 9, 2013, 1:40 p.m.
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.
Lei Li - Jan. 11, 2013, 8 a.m.
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...:(

>
Lei Li - Jan. 11, 2013, 8:03 a.m.
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.
>

Patch

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.