On Tue, Apr 02, 2013 at 07:47:22PM +0800, Wenchao Xia wrote:
> diff --git a/qmp-commands.hx b/qmp-commands.hx> index 1e0e11e..6b20684 100644> --- a/qmp-commands.hx> +++ b/qmp-commands.hx> @@ -1765,6 +1765,61 @@ EQMP> },> > SQMP> +query-snapshots> +---------------> +> +Show the internal consistent snapshot information> +> +Each snapshot is represented by a json-object. The returned value> +is a json-array of all snapshots
I think the "consistent snapshot" term is not defined. Please add
something like:
"Consistent snapshots are snapshots that exist in all writeable block
devices. When 'savevm' takes a snapshot it uses the same ID across all
writeable block devices. If a snapshot ID only exists in one block
device then it is not a consistent snapshot."
Stefan

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.
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.

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.

于 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.

On 04/11/2013 09:17 PM, Wenchao Xia wrote:
>>>>> 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.
Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,
changing that output to delete a field might break existing tools;
adding a field might be safer, but then you'd have redundant information
in the old style for old tools, as well as the added field.

于 2013-4-12 11:22, Eric Blake 写道:
> On 04/11/2013 09:17 PM, Wenchao Xia wrote:>>>>>>>> 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.>> Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,> changing that output to delete a field might break existing tools;> adding a field might be safer, but then you'd have redundant information> in the old style for old tools, as well as the added field.>
It seems keeping it, is what best we can do now. Markus, are you OK
with it?

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-12 11:22, Eric Blake 写道:>> On 04/11/2013 09:17 PM, Wenchao Xia wrote:>>>>>>>>>>> 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.>>>> Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,>> changing that output to delete a field might break existing tools;>> adding a field might be safer, but then you'd have redundant information>> in the old style for old tools, as well as the added field.>>> It seems keeping it, is what best we can do now. Markus, are you OK> with it?
Yes, because:
"qemu-img info --output=json" exists since commit c054b3f, v1.3.0. I
agree we shouldn't change it incompatibly just because we hate the way
time is represented there.
Doesn't mean we *have* to duplicate the ugly time representation into
QMP. But avoiding it there involves duplicating SnapshotInfo and
ImageInfo in the schema. Not worth it, considering there's no
consistency in QMP's time representation anyway.