From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48240) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsL9p-0001kM-TD for qemu-devel@nongnu.org; Mon, 07 Jan 2013 17:27:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsL9m-0002LO-6O for qemu-devel@nongnu.org; Mon, 07 Jan 2013 17:27:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:20363) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsL9l-0002LE-V2 for qemu-devel@nongnu.org; Mon, 07 Jan 2013 17:26:58 -0500 Message-ID: <50EB4BAC.7060908@redhat.com> Date: Mon, 07 Jan 2013 15:26:52 -0700 From: Eric Blake MIME-Version: 1.0 References: <1357466820-12860-1-git-send-email-lilei@linux.vnet.ibm.com> <1357466820-12860-4-git-send-email-lilei@linux.vnet.ibm.com> In-Reply-To: <1357466820-12860-4-git-send-email-lilei@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig90E6433B5B62C7589D4DF087" Subject: Re: [Qemu-devel] [PATCH 3/3] qga: add guest-set-time command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lei Li Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig90E6433B5B62C7589D4DF087 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 01/06/2013 03:07 AM, Lei Li wrote: > Signed-off-by: Lei Li > --- > qga/commands-posix.c | 57 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++ > 2 files changed, 89 insertions(+), 0 deletions(-) >=20 > 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 **er= rp) > return host_time; > } > =20 > +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 =3D get_host_time(); > + if (!host_time) { > + error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to set gu= est time"); > + return; > + } > + tv.tv_sec =3D host_time->seconds; > + tv.tv_usec =3D host_time->microseconds; > + } else if (has_seconds && has_microseconds && has_utc_offset) { > + tv.tv_sec =3D (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 =3D (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 =3D seconds + (has_utc_offset ? utc_offset : 0); ... } > +++ b/qga/qapi-schema.json > @@ -117,6 +117,38 @@ > 'returns': 'HostTimeInfo' } > =20 > ## > +# @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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig90E6433B5B62C7589D4DF087 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with undefined - http://www.enigmail.net/ iQEcBAEBCAAGBQJQ60usAAoJEKeha0olJ0Nql7sH/igkHcfgMw6JU37JSKYGdPs6 0/OXaI7gpJ55sxaXBvhOchDkobEzAdYdPt3CZ+TNFd4IcWeFWtgwcunaFcIP8hjS XJ5CK4AOpAQotCBkRyRztgTytmDga/Ckir/bsxpdV8GX4ULvmssETqMOCRHOHaj5 Y50D4EANgYqEFqG8/iyCLQ6gryu21OCydaV6k5BK6Z3sbtUMMqfmf0wT1X/Q6w7i jElIHqIHREHkaXt0Wg50pHDKgPJ8b2eVIeFNK3h+yxRvZrPUK8cG0VdwgXOXbMuT syODDcIQTLhr98myAaBzFammzROJ1R1QDc0R+BIJbDKGEyJ2hjnML2H1J6hN5nI= =GKAk -----END PGP SIGNATURE----- --------------enig90E6433B5B62C7589D4DF087--