Re: [Qemu-devel] [PATCH v2] qga: Don't require 'time' argument in guest-set-time command

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Michal Privoznik (2014-01-31 04:29:51)
> As the description to the guest-set-time states, the command is
> there to ease time synchronization after resume. If guest was
> suspended for longer period of time, its system time can go off
> so badly, that even NTP refuses to set it. That's why the command
> was invented: to give users chance to set the time (not
> necessarily 100% correct). However, there's is no real need for
> us to require users to pass an arbitrary time. Especially if we
> can read the correct value from RTC (boiling down to reading
> host's time). Hence this commit enables logic:
> 
> guest-set-time() == guest-set-time($now_from_rtc)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Thanks, applied to qga tree:                                                                                 

  https://github.com/mdroth/qemu/commits/qga

> ---
> diff to v1:
> -Fix checkpatch.pl warnings 
> 
>  qga/commands-posix.c | 41 +++++++++++++++++++++++++----------------
>  qga/commands-win32.c | 34 +++++++++++++++++++++++-----------
>  qga/qapi-schema.json |  9 +++++----
>  3 files changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8100bee..7d6665a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -142,7 +142,7 @@ int64_t qmp_guest_get_time(Error **errp)
>     return time_ns;
>  }
> 
> -void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>  {
>      int ret;
>      int status;
> @@ -150,22 +150,28 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
>      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);
> -        return;
> +    /* If user has passed a time, validate and set it. */
> +    if (has_time) {
> +        /* 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);
> +            return;
> +        }
> +
> +        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;
> +        }
>      }
> 
> -    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. */
> +    /* Now, if user has passed a time to set and the system time is set, we
> +     * just need to synchronize the hardware clock. However, if no time was
> +     * passed, user is requesting the opposite: set the system time from the
> +     * hardware clock. */
>      pid = fork();
>      if (pid == 0) {
>          setsid();
> @@ -173,7 +179,10 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
>          reopen_fd_to_null(1);
>          reopen_fd_to_null(2);
> 
> -        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
> +        /* Use '/sbin/hwclock -w' to set RTC from the system time,
> +         * or '/sbin/hwclock -s' to set the system time from RTC. */
> +        execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
> +               NULL, environ);
>          _exit(EXIT_FAILURE);
>      } else if (pid < 0) {
>          error_setg_errno(errp, errno, "failed to create child process");
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index a6a0af2..8245f95 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -370,25 +370,37 @@ int64_t qmp_guest_get_time(Error **errp)
>      return time_ns;
>  }
> 
> -void qmp_guest_set_time(int64_t time_ns, Error **errp)
> +void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>  {
>      SYSTEMTIME ts;
>      FILETIME tf;
>      LONGLONG time;
> 
> -    if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> -        error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> -        return;
> -    }
> +    if (has_time) {
> +        /* Okay, user passed a time to set. Validate it. */
> +        if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
> +            error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
> +            return;
> +        }
> 
> -    time = time_ns / 100 + W32_FT_OFFSET;
> +        time = time_ns / 100 + W32_FT_OFFSET;
> 
> -    tf.dwLowDateTime = (DWORD) time;
> -    tf.dwHighDateTime = (DWORD) (time >> 32);
> +        tf.dwLowDateTime = (DWORD) time;
> +        tf.dwHighDateTime = (DWORD) (time >> 32);
> 
> -    if (!FileTimeToSystemTime(&tf, &ts)) {
> -        error_setg(errp, "Failed to convert system time %d", (int)GetLastError());
> -        return;
> +        if (!FileTimeToSystemTime(&tf, &ts)) {
> +            error_setg(errp, "Failed to convert system time %d",
> +                       (int)GetLastError());
> +            return;
> +        }
> +    } else {
> +        /* Otherwise read the time from RTC which contains the correct value.
> +         * Hopefully. */
> +        GetSystemTime(&ts);
> +        if (ts.wYear < 1601 || ts.wYear > 30827) {
> +            error_setg(errp, "Failed to get time");
> +            return;
> +        }
>      }
> 
>      acquire_privilege(SE_SYSTEMTIME_NAME, errp);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 245f968..80edca1 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -120,17 +120,18 @@
>  # This command tries to set guest time to the given value,
>  # then sets the Hardware Clock to the current System Time.
>  # This will make it easier for a guest to resynchronize
> -# without waiting for NTP.
> +# without waiting for NTP. If no @time is specified, then
> +# the time to set is read from RTC.
>  #
> -# @time: time of nanoseconds, relative to the Epoch of
> -#        1970-01-01 in UTC.
> +# @time: #optional time of nanoseconds, relative to the Epoch
> +#        of 1970-01-01 in UTC.
>  #
>  # Returns: Nothing on success.
>  #
>  # Since: 1.5
>  ##
>  { 'command': 'guest-set-time',
> -  'data': { 'time': 'int' } }
> +  'data': { '*time': 'int' } }
> 
>  ##
>  # @GuestAgentCommandInfo:
> -- 
> 1.8.5.2