Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4208 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]