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

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

On Mon, Mar 04, 2013 at 05:16:30PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6fc6003..0515f5f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -135,6 +135,60 @@ 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 (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
> +        error_setg(errp, "Time %" PRId64 " is too large", 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 52bb091..ce964e9 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -96,6 +96,33 @@
>    'returns': 'int' }
> 
>  ##
> +# @guest-set-time:
> +#
> +# Set guest time.
> +#
> +# 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

"time in nanoseconds", but I can fix this up myself if there are no
other comments that need to be addressed.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> +#        1970-01-01 in UTC.
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'guest-set-time',
> +  'data': { 'time': 'int' } }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
> -- 
> 1.7.11.7
>

On 03/04/2013 02:16 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6fc6003..0515f5f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -135,6 +135,60 @@ 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 (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
> +        error_setg(errp, "Time %" PRId64 " is too large", time_ns);

You set the error, but then fall through...

> +    }
> +
> +    tv.tv_sec = time_ns / 1000000000;
> +    tv.tv_usec = (time_ns % 1000000000) / 1000;
> +
> +    ret = settimeofday(&tv, NULL);

...and change the guest time to an invalid value anyways.  This could
cause the guest to severely misbehave.

> +    if (ret < 0) {
> +        error_setg_errno(errp, errno, "Failed to set time to guest");
> +        return;

You should have done an early exit on the other error, like you did here.

> +#
> +# This command tries to set guest time based on the information
> +# from host or an absolute value given by management app, and

The absolute value given by the management app _is_ information from the
host.  I'd simplify this to just:

This command tries to set guest time to the given value, then

> +# set the Hardware Clock to the current System Time. This

s/set/sets/

> +# 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.
> +#

On 03/04/2013 11:16 AM, mdroth wrote:
> On Mon, Mar 04, 2013 at 05:16:30PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qga/qapi-schema.json | 27 ++++++++++++++++++++++++++
>>  2 files changed, 81 insertions(+)

>>
>> +# @time: time of nanoseconds, relative to the Epoch of
> 
> "time in nanoseconds", but I can fix this up myself if there are no
> other comments that need to be addressed.

Unfortunately, that's not the only problem, and I think a v5 is
warranted (see my other mail).