Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore

[Kevin Wolf]

Am 11.01.2019 um 14:22 hat Max Reitz geschrieben:
> On 10.01.19 12:41, Dr. David Alan Gilbert wrote:
> > * Kevin Wolf (address@hidden) wrote:
> >> Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:
> >>> On 1/9/19 12:51 PM, Kevin Wolf wrote:
> >>>
> >>>>> Indeed, and libvirt IS using 'savevm' via HMP via QMP's
> >>>>> human-monitor-command, since there is no QMP counterpart for internal
> >>>>> snapshot.  Even though lately we consistently tell people that internal
> >>>>> snapshots are underdeveloped and you should use external snapshots, it
> >>>>> does not get away from the fact that libvirt has been using 'savevm' to
> >>>>> drive internal snapshots for years now, and that we MUST consider
> >>>>> back-compat and/or add an introspectible QMP interface before making
> >>>>> changes that would break libvirt.
> >>>>
> >>>> Okay, so what does libvirt do when you request a snapshot with a
> >>>> numerical name? Without having looked at the code, the best case I would
> >>>> expect that it forbids them, and more realistically I suspect that we
> >>>> may actually fix a bug for libvirt by changing the semantics.
> >>>>
> >>>> Or does libvirt really use snapshot IDs rather than names?
> >>>
> >>> At the moment, libvirt does not place any restriction on internal
> >>> snapshot names, but passes the user's name through without thought of
> >>> whether it is an id or a name.
> >>>
> >>> So yes, arguably tightening the semantics fixes a libvirt bug for
> >>> libvirt having allowed internal snapshots to get into an inconsistent
> >>> state.
> >>
> >> So there are two scenarios to consider with respect to breaking
> >> backwards compatibility:
> >>
> >> 1. There may be code out there that relies on numeric arguments being
> >>    interpreted as IDs. This code will break if we make this change and
> >>    numeric snapshot names exist. That such code exists is speculation
> >>    (even though plausible) and we don't know how widespread it is.
> >>
> >> 2. There may be code out there that blindly assumes that whatever it
> >>    passes is interpreted as a name. Nobody considered that with a
> >>    numeric snapshot name, it maybe misinterpreted as an ID. We know that
> >>    this is true for libvirt, the single most used management tool for
> >>    QEMU. More code like this may (and probably does) exist.
> >>
> >> Essentially the same two categories exist for human users: those who
> >> somehow found out that QEMU accepts IDs as monitor command arguments and
> >> are using those (mitigated by not displaying IDs any more), and those
> >> who are trapped because they wanted to access a numeric name, but
> >> surprisingly got it interpreted as an ID. Both are speculation to some
> >> degree, but my guess is that the latter group is larger.
> >>
> >> Given all this, this is a no-brainer for me. We simplify the interface,
> >> we simplify the code, we make things less confusing for manual users and
> >> we fix the management tool that everybody uses. How could we not make
> >> this change?
> 
> So you're trying to make a bug fix out of this now to get around
> deprecation?  To me, changing behavior in qemu to fix a bug in libvirt
> doesn't equate to fixing a bug in qemu.  So let's try to find a real bug
> in qemu.

I'm not making this a bug fix, but a interface cleanup that fixes a bug
as a side effect, which makes it only more appealing. I'm also not sure
what exactly you mean by "get around deprecation".

If you mean the formal two releases deprecation period that is required
by our deprecation policy, it doesn't apply because this is HMP and not
a stable interface.

If you mean deprecation not because we must but because we're
considering actual users, then I have described how making the change
fixes things for more users than it could potentially break things. In
addition, I pointed out how a deprecation period is useless for
user-facing changes. I'm concluding that a "voluntary" deprecation for
two releases isn't helpful at all.

> >>> But again, it falls in the category of "properly fixing this
> >>> requires a lot of auditing, time, mental power, and unit tests", which
> >>> makes it a rather daunting task to state for certain.
> >>
> >> Fix the world before we can fix anything?
> > 
> > Can't we break this loop for the savevm command fairly easily; it's
> > currently documnted as:
> > 
> > savevm [tag|id]
> 
> The bug starts here.
> [... snipped long description of how horrible everything is ...]

Yes, it's horrible. Let's radically simplify the interface (and with it
the code) by throwing away the useless ID stuff. Which happens to be
exactly what this series was getting at.

There is no point in spending time for changing the current interface to
make a little more sense while keeping its fundamental insanity
unaddressed, then deprecate it, and then change it again to _actually_
make sense. Let's convert it directly to something that makes sense.

> So, yes, there are bugs in qemu, but to fix them, we would need to
> switch the comparison order in bdrv_snapshot_find() and fix
> bdrv_snapshot_delete_by_id_or_name() to always invoke
> bdrv_snapshot_delete() for both cases (ID and tag).  And we should
> probably remove the special code path in savevm for overwriting an
> existing snapshot.
> 
> This would also alleviate the bug in libvirt (because the tag then
> always takes precedence, like the interface description suggests).

Yes, at least tools which don't use IDs wouldn't be broken any more. But
you removed none of the insanity. The same way addressing by tag was
broken before, addressing by ID is broken now (previously, you need to
check first that no ID exists while you want to address a tag; now, you
need to check first that no tag exists while you want to address an ID).

In fact, if you're willing to go there, you break exactly the same users
that would be broken if we dropped IDs from the interface completely. So
why not do the full thing instead of stopping halfway? You're taking the
potential problems without the advantages you could get.

> Sure, this series would fix the bugs as well, but it does more than
> that.  It really isn't just a bug fix.

It was never meant to be a bug fix (unless, of course, you consider
insane interfaces a bug). But that it fixes a bug as a side effect
contributes to the fact that breaking compatibility has not only
downsides and that a deprecation period would be useless or even
counterproductive.

Kevin

signature.asc
Description: PGP signature