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

[Luiz Capitulino <lcapitulino@redhat.com>]

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.