qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
Date: Fri, 12 Apr 2013 11:17:02 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130328 Thunderbird/17.0.5

于 2013-4-11 21:39, Luiz Capitulino 写道:
On Thu, 11 Apr 2013 07:08:41 -0600
Eric Blake <address@hidden> 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]