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

login
register
mail settings
Submitter Lei Li
Date March 1, 2013, 9:33 a.m.
Message ID <1362130380-1895-3-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/224264/
State New
Headers show

Comments

Lei Li - March 1, 2013, 9:33 a.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)
Eric Blake - March 1, 2013, 5:38 p.m.
On 03/01/2013 02:33 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f159e25..e246a0d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -135,6 +135,61 @@ int64_t qmp_guest_get_time(Error **errp)
>     return time_ns; 
>  }
>  
> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +{
> +    int ret;
> +    int status;
> +    pid_t pid;
> +    Error *local_err = NULL;
> +    struct timeval tv;
> +
> +    /* year-2038 will overflow in case time_t is 32bit */
> +    if ((sizeof(time_t) <= 4) && ((unsigned long) time_ns & (1ul << 31))) {

Wrong.  If 'unsigned long' and 'time_t' are both 32 bits, but time_ns is
0x1000000000000000, then the cast truncates to 0 and you don't report
overflow.  Conversely, if time_ns is 0x80000000, you report overflow,
even though this value fits in 32-bit time_t after you do division from
nanoseconds back to seconds.  What you WANT is:

if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {

> +        error_setg_errno(errp, errno, "Invalid time %ld for overflow",
> +                         time_ns);

That wording sounds awkward.  Worse, errno is NOT set to anything sane,
so you do NOT want error_setg_errno.  And %ld is wrong for int64_t on
32-bit platforms.  How about:

error_setg(errp, "Time %" PRI64D " is too large", time_ns);

> +++ b/qga/qapi-schema.json
> @@ -99,6 +99,33 @@
>    'returns': 'int' }
>  
>  ##
> +# @guest-set-time:
> +#
> +# Set guest time.
> +#
> +# Right now, when a guest is paused or migrated to a file

s/Right now, when/When/

> +# 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.
> +#
> +# @time: time of nanoseconds, relative to the Epoch of
> +#        1970-01-01 in UTC/GMT.

drop '/GMT'

> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'guest-set-time',
> +  'data': { 'time': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
>
Eric Blake - March 1, 2013, 5:50 p.m.
On 03/01/2013 10:38 AM, Eric Blake wrote:
>> +        error_setg_errno(errp, errno, "Invalid time %ld for overflow",
>> +                         time_ns);
> 
> That wording sounds awkward.  Worse, errno is NOT set to anything sane,
> so you do NOT want error_setg_errno.  And %ld is wrong for int64_t on
> 32-bit platforms.  How about:
> 
> error_setg(errp, "Time %" PRI64D " is too large", time_ns);

Don't know what I was thinking; there is no PRI64D.  I meant PRId64.
Lei Li - March 4, 2013, 8:25 a.m.
On 03/02/2013 01:38 AM, Eric Blake wrote:
> On 03/01/2013 02:33 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
>>   2 files changed, 82 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index f159e25..e246a0d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -135,6 +135,61 @@ int64_t qmp_guest_get_time(Error **errp)
>>      return time_ns;
>>   }
>>   
>> +void qmp_guest_set_time(int64_t time_ns, Error **errp)
>> +{
>> +    int ret;
>> +    int status;
>> +    pid_t pid;
>> +    Error *local_err = NULL;
>> +    struct timeval tv;
>> +
>> +    /* year-2038 will overflow in case time_t is 32bit */
>> +    if ((sizeof(time_t) <= 4) && ((unsigned long) time_ns & (1ul << 31))) {
> Wrong.  If 'unsigned long' and 'time_t' are both 32 bits, but time_ns is
> 0x1000000000000000, then the cast truncates to 0 and you don't report
> overflow.  Conversely, if time_ns is 0x80000000, you report overflow,
> even though this value fits in 32-bit time_t after you do division from
> nanoseconds back to seconds.  What you WANT is:

Yes, you are right.. thanks.

>
> if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
>
>> +        error_setg_errno(errp, errno, "Invalid time %ld for overflow",
>> +                         time_ns);
> That wording sounds awkward.  Worse, errno is NOT set to anything sane,
> so you do NOT want error_setg_errno.  And %ld is wrong for int64_t on
> 32-bit platforms.  How about:
>
> error_setg(errp, "Time %" PRI64D " is too large", time_ns);

Sure, will address it as the your later reply.

>> +++ b/qga/qapi-schema.json
>> @@ -99,6 +99,33 @@
>>     'returns': 'int' }
>>   
>>   ##
>> +# @guest-set-time:
>> +#
>> +# Set guest time.
>> +#
>> +# Right now, when a guest is paused or migrated to a file
> s/Right now, when/When/
>
>> +# 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.
>> +#
>> +# @time: time of nanoseconds, relative to the Epoch of
>> +#        1970-01-01 in UTC/GMT.
> drop '/GMT'
>
>> +#
>> +# Returns: Nothing on success.
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'guest-set-time',
>> +  'data': { 'time': 'int' } }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
>>

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index f159e25..e246a0d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -135,6 +135,61 @@  int64_t qmp_guest_get_time(Error **errp)
    return time_ns; 
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    int ret;
+    int status;
+    pid_t pid;
+    Error *local_err = NULL;
+    struct timeval tv;
+
+    /* year-2038 will overflow in case time_t is 32bit */
+    if ((sizeof(time_t) <= 4) && ((unsigned long) time_ns & (1ul << 31))) {
+        error_setg_errno(errp, errno, "Invalid time %ld for overflow",
+                         time_ns);
+    }
+
+    tv.tv_sec = time_ns / 1000000000;
+    tv.tv_usec = (time_ns % 1000000000) / 1000;
+
+    ret = settimeofday(&tv, NULL);
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        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) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        return;
+    }
+
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!WIFEXITED(status)) {
+        error_setg(errp, "child process has terminated abnormally");
+        return;
+    }
+
+    if (WEXITSTATUS(status)) {
+        error_setg(errp, "hwclock failed to set hardware clock to system time");
+        return;
+    }
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 563600c..5eba324 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -99,6 +99,33 @@ 
   'returns': 'int' }
 
 ##
+# @guest-set-time:
+#
+# Set guest time.
+#
+# 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.
+#
+# @time: time of nanoseconds, relative to the Epoch of
+#        1970-01-01 in UTC/GMT.
+#
+# Returns: Nothing on success.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-set-time',
+  'data': { 'time': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.