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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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