From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQUV7-0002tI-Kd for qemu-devel@nongnu.org; Thu, 11 Apr 2013 23:18:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UQUV6-0008ML-C5 for qemu-devel@nongnu.org; Thu, 11 Apr 2013 23:18:09 -0400 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:57803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQUV5-0008L0-MN for qemu-devel@nongnu.org; Thu, 11 Apr 2013 23:18:08 -0400 Received: from /spool/local by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 12 Apr 2013 08:43:13 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id D3DFCE0053 for ; Fri, 12 Apr 2013 08:49:49 +0530 (IST) Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3C3HsJp2556262 for ; Fri, 12 Apr 2013 08:47:55 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3C3HvQE001317 for ; Fri, 12 Apr 2013 13:17:57 +1000 Message-ID: <51677CAE.5090805@linux.vnet.ibm.com> Date: Fri, 12 Apr 2013 11:17:02 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1364903250-10429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1364903250-10429-10-git-send-email-xiawenc@linux.vnet.ibm.com> <878v4qxu1z.fsf@blackfin.pond.sub.org> <5166521E.8060201@linux.vnet.ibm.com> <87bo9lfes4.fsf@blackfin.pond.sub.org> <20130411084401.47405e80@redhat.com> <5166B5D9.1010906@redhat.com> <20130411093914.24941714@redhat.com> In-Reply-To: <20130411093914.24941714@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, stefanha@gmail.com, Markus Armbruster , qemu-devel@nongnu.org, pbonzini@redhat.com 于 2013-4-11 21:39, Luiz Capitulino 写道: > On Thu, 11 Apr 2013 07:08:41 -0600 > Eric Blake wrote: > >> On 04/11/2013 06:44 AM, Luiz Capitulino wrote: >> >>>>>>> +-> { "execute": "query-snapshots" } >>>>>>> +<- { >>>>>>> + "return":[ >>>>>>> + { >>>>>>> + "id": "1", >>>>>>> + "name": "snapshot1", >>>>>>> + "vm-state-size": 0, >>>>>>> + "date-sec": 10000200, >>>>>>> + "date-nsec": 12, >>>>>>> + "vm-clock-sec": 206, >>>>>>> + "vm-clock-nsec": 30 >>>>>> >>>>>> Not your patch's fault, but here goes anyway: I dislike this >>>>>> representation of time. >>>>>> >>>>>> QMP has time in seconds, milliseconds, nanoseconds, (seconds, >> >>> Are you saying you're going to drop vm-clock-sec? >>> >>>> Before you do that, let's get Luiz's blessing. >>> >>> Fine with me if I got it right. >> >> Problem. Let's go back to the original definition that we are talking >> about modifying: >> >> +# >> +# Since: 1.5 >> +## >> +{ 'command': 'query-snapshots', >> + 'returns': ['SnapshotInfo'] } >> + >> >> Now let's look for that type: >> >> # @vm-clock-sec: VM clock relative to boot in seconds >> # >> # @vm-clock-nsec: fractional part in nano seconds to be used with >> vm-clock-sec >> # >> # Since: 1.3 >> # >> ## >> >> { 'type': 'SnapshotInfo', >> 'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int', >> 'date-sec': 'int', 'date-nsec': 'int', >> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } } >> >> >> And that type is already used in 'ImageInfo'. >> >> In other words, we're stuck. We've already cemented the mistake. You >> _can't_ drop vm-clock-sec, without breaking the behavior of management >> apps written against the 1.3 interface when dealing with ImageInfo. If >> you want to avoid a (sec/nsec) pair, you would have to invent a new type >> instead of reusing the already-existing SnapshotInfo. > > You're right. I actually replied too quickly and thought that we were talking > about the types being introduced by this patch. Sorry for that. > >> Hmm, as I typed that, I did another search of qemu-schema.json - we have >> the type 'ImageInfo' defined, but none of the existing 'command's ever >> call out the use of that type. Is it a type we are only using >> internally to date, and where this is the first QMP command that would >> actually expose SnapshotInfo or ImageInfo to a management app? Then >> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and >> provide a saner type for the first time that QMP can actually use it. > > IIRC, it's being used by qemu-img. As it's not trivial to determine if > the field has users and as what goes in the schema is to be considered stable, > it's better to keep the field. Instead, we could add a better field and > deprecate the current one. > I remember there is a patch laster year made "qemu-img info" dump out json strings, where vm-clock-sec is dumped out. -- Best Regards Wenchao Xia