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

login
register
mail settings
Submitter Lei Li
Date Jan. 27, 2013, 6:14 p.m.
Message ID <1359310460-23564-3-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/216026/
State New
Headers show

Comments

Lei Li - Jan. 27, 2013, 6:14 p.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json |   42 +++++++++++++++++++++++++++
 2 files changed, 118 insertions(+), 0 deletions(-)
Eric Blake - Jan. 28, 2013, 8:45 p.m.
On 01/27/2013 11:14 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json |   42 +++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 2fef2b6..5424c50 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -149,6 +149,82 @@ struct TimeInfo *qmp_guest_get_time(Error **errp)
>      return time_info;
>  }
>  
> +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;
> +    TimeInfo *time_info;
> +
> +    /* year-2038 will overflow in case time_t is 32bit */
> +    if (sizeof(time_t) == 4) {
> +        return;
> +    }

What? Silently giving up, without even seeing whether overflow happened?
 That just feels wrong.  Make the change if it is in range, and be vocal
about setting an error if there is overflow.

> +
> +    if (!has_seconds) {
> +        if (has_microseconds || has_utc_offset) {
> +            error_setg(errp, "field 'seconds' missing");
> +            return;
> +        } else {
> +            /* Grab time from the host if no arguments given. */
> +            time_info = get_host_time(errp);

Does this even make sense?  Since this code will be executing in the
guest, you are getting the guest's current notion of time (as I said in
1/2, get_host_time() is misnamed); and then setting the guest's time
right back to the same value.  Why bother?

It seems like if the host is going to bother issuing the guest-set-time
agent command, then the host should have a mandatory argument to tell
the guest what to set its time to.

Then again, reading the rest of your code, I can see one case where it
_might_ make sense to allow the host to not pass in a time - namely, if
the host wants to force the guest to call hwclock to write its notion of
current system time (in RAM) back to the hardware.  But that seems like
a stretch.

> +            if (!time_info) {
> +                return;
> +            }
> +
> +            tv.tv_sec = time_info->seconds;
> +            tv.tv_usec = time_info->microseconds;
> +        }
> +    } else {
> +        if (abs(utc_offset) > (24 * 60)) {

Won't work.  abs() takes an int argument, so you can pass in int64_t
values that will overflow the range of int and not be detected by this
overflow check.

> +            error_setg(errp, "'utc_offset' shoud be less than 24 hours");

s/shoud/should/

> +            return;
> +        }
> +
> +        if (microseconds > 1000000) {
> +            error_setg(errp, "'microseconds' overflow");
> +            return;
> +        }
> +
> +        tv.tv_sec = (time_t) seconds + (has_utc_offset ? utc_offset * 60 : 0);
> +        tv.tv_usec = has_microseconds ? (time_t) microseconds : 0;
> +    }
> +
> +
> +    ret = settimeofday(&tv, NULL);
> +    if (ret < 0) {
> +        error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));

Prefer 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);
> +        _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);

error_setg and a nicer message would be nicer to the end user.

> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index d067fa5..6eba625 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -121,6 +121,48 @@
>    'returns': 'TimeInfo' }
>  
>  ##
> +# @guest-set-time:
> +#
> +# Set guest time. If no arguments were given, will set
> +# host time to guest.

I don't see how omitted time arguments can possibly work.  It makes
sense from a management app point of view (tell me a time, and I'll call
guest-set-time with that value on your behalf; or don't tell me a time,
I'll get the host time, and then call guest-set-time with the right
value); but this file is not documenting the management app, but the
JSON interface that the guest sees.  In other words, I think time has to
be a mandatory argument by the time you get this low in the stack.

> +#
> +# 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.
> +#
> +# Notes: If no arguments were given, will grab time from
> +#        the host.
> +#
> +#        @microseconds and @utc-offset must not be provided
> +#        unless @seconds is present.
> +#
> +#        If just @seconds provided, other fields would be
> +#        set to zero.
> +#
> +# present
> +# Since: 1.4
> +##
> +{ 'command': 'guest-set-time',
> +  'data': { '*seconds': 'int', '*microseconds': 'int',
> +            '*utc-offset': 'int' } }

Why not just call out 'data': 'TimeInfo', especially if we make the time
argument mandatory at this level?  And see Anthony's comments on 1/2,
that you really don't need utc-offset here; passing a single int64_t of
nanoseconds relative to the Epoch in UTC, and forcing the guest to
translate that according to the guest's time zone, does seem simpler on
the protocol (maybe more work for the management app in generating the
number to send over the protocol, and more work for the guest to
interpret that number, but that's the engineering tradeoff for a simpler
interface).
Lei Li - Jan. 30, 2013, 7:46 a.m.
On 01/29/2013 04:45 AM, Eric Blake wrote:
> On 01/27/2013 11:14 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/qapi-schema.json |   42 +++++++++++++++++++++++++++
>>   2 files changed, 118 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 2fef2b6..5424c50 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -149,6 +149,82 @@ struct TimeInfo *qmp_guest_get_time(Error **errp)
>>       return time_info;
>>   }
>>   
>> +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;
>> +    TimeInfo *time_info;
>> +
>> +    /* year-2038 will overflow in case time_t is 32bit */
>> +    if (sizeof(time_t) == 4) {
>> +        return;
>> +    }
> What? Silently giving up, without even seeing whether overflow happened?
>   That just feels wrong.  Make the change if it is in range, and be vocal
> about setting an error if there is overflow.

Yes, this is my stupid mistake...

>
>> +
>> +    if (!has_seconds) {
>> +        if (has_microseconds || has_utc_offset) {
>> +            error_setg(errp, "field 'seconds' missing");
>> +            return;
>> +        } else {
>> +            /* Grab time from the host if no arguments given. */
>> +            time_info = get_host_time(errp);
> Does this even make sense?  Since this code will be executing in the
> guest, you are getting the guest's current notion of time (as I said in
> 1/2, get_host_time() is misnamed); and then setting the guest's time
> right back to the same value.  Why bother?
>
> It seems like if the host is going to bother issuing the guest-set-time
> agent command, then the host should have a mandatory argument to tell
> the guest what to set its time to.
>
> Then again, reading the rest of your code, I can see one case where it
> _might_ make sense to allow the host to not pass in a time - namely, if
> the host wants to force the guest to call hwclock to write its notion of
> current system time (in RAM) back to the hardware.  But that seems like
> a stretch.
>
>> +            if (!time_info) {
>> +                return;
>> +            }
>> +
>> +            tv.tv_sec = time_info->seconds;
>> +            tv.tv_usec = time_info->microseconds;
>> +        }
>> +    } else {
>> +        if (abs(utc_offset) > (24 * 60)) {
> Won't work.  abs() takes an int argument, so you can pass in int64_t
> values that will overflow the range of int and not be detected by this
> overflow check.

Yes, you are right.

>> +            error_setg(errp, "'utc_offset' shoud be less than 24 hours");
> s/shoud/should/
>
>> +            return;
>> +        }
>> +
>> +        if (microseconds > 1000000) {
>> +            error_setg(errp, "'microseconds' overflow");
>> +            return;
>> +        }
>> +
>> +        tv.tv_sec = (time_t) seconds + (has_utc_offset ? utc_offset * 60 : 0);
>> +        tv.tv_usec = has_microseconds ? (time_t) microseconds : 0;
>> +    }
>> +
>> +
>> +    ret = settimeofday(&tv, NULL);
>> +    if (ret < 0) {
>> +        error_set(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));
> Prefer 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);
>> +        _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);
> error_setg and a nicer message would be nicer to the end user.
>
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index d067fa5..6eba625 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -121,6 +121,48 @@
>>     'returns': 'TimeInfo' }
>>   
>>   ##
>> +# @guest-set-time:
>> +#
>> +# Set guest time. If no arguments were given, will set
>> +# host time to guest.
> I don't see how omitted time arguments can possibly work.  It makes
> sense from a management app point of view (tell me a time, and I'll call
> guest-set-time with that value on your behalf; or don't tell me a time,
> I'll get the host time, and then call guest-set-time with the right

Sorry, I think I am supposed to make it work like this way?

> value); but this file is not documenting the management app, but the
> JSON interface that the guest sees.  In other words, I think time has to
> be a mandatory argument by the time you get this low in the stack.
>
>> +#
>> +# 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.
>> +#
>> +# Notes: If no arguments were given, will grab time from
>> +#        the host.
>> +#
>> +#        @microseconds and @utc-offset must not be provided
>> +#        unless @seconds is present.
>> +#
>> +#        If just @seconds provided, other fields would be
>> +#        set to zero.
>> +#
>> +# present
>> +# Since: 1.4
>> +##
>> +{ 'command': 'guest-set-time',
>> +  'data': { '*seconds': 'int', '*microseconds': 'int',
>> +            '*utc-offset': 'int' } }
> Why not just call out 'data': 'TimeInfo', especially if we make the time
> argument mandatory at this level?  And see Anthony's comments on 1/2,
> that you really don't need utc-offset here; passing a single int64_t of
> nanoseconds relative to the Epoch in UTC, and forcing the guest to
> translate that according to the guest's time zone, does seem simpler on
> the protocol (maybe more work for the management app in generating the
> number to send over the protocol, and more work for the guest to
> interpret that number, but that's the engineering tradeoff for a simpler
> interface).
>

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2fef2b6..5424c50 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -149,6 +149,82 @@  struct TimeInfo *qmp_guest_get_time(Error **errp)
     return time_info;
 }
 
+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;
+    TimeInfo *time_info;
+
+    /* year-2038 will overflow in case time_t is 32bit */
+    if (sizeof(time_t) == 4) {
+        return;
+    }
+
+    if (!has_seconds) {
+        if (has_microseconds || has_utc_offset) {
+            error_setg(errp, "field 'seconds' missing");
+            return;
+        } else {
+            /* Grab time from the host if no arguments given. */
+            time_info = get_host_time(errp);
+            if (!time_info) {
+                return;
+            }
+
+            tv.tv_sec = time_info->seconds;
+            tv.tv_usec = time_info->microseconds;
+        }
+    } else {
+        if (abs(utc_offset) > (24 * 60)) {
+            error_setg(errp, "'utc_offset' shoud be less than 24 hours");
+            return;
+        }
+
+        if (microseconds > 1000000) {
+            error_setg(errp, "'microseconds' overflow");
+            return;
+        }
+
+        tv.tv_sec = (time_t) seconds + (has_utc_offset ? utc_offset * 60 : 0);
+        tv.tv_usec = has_microseconds ? (time_t) microseconds : 0;
+    }
+
+
+    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 d067fa5..6eba625 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -121,6 +121,48 @@ 
   'returns': 'TimeInfo' }
 
 ##
+# @guest-set-time:
+#
+# Set guest time. If no 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.
+#
+# Notes: If no arguments were given, will grab time from
+#        the host.
+#
+#        @microseconds and @utc-offset must not be provided
+#        unless @seconds is present.
+#
+#        If just @seconds provided, other fields would be
+#        set to zero.
+#
+# present
+# Since: 1.4
+##
+{ 'command': 'guest-set-time',
+  'data': { '*seconds': 'int', '*microseconds': 'int',
+            '*utc-offset': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.