qemu-devel
[Top][All Lists]
Advanced

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

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore
Date: Fri, 11 Jan 2019 16:23:38 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0

On 11.01.19 15:33, Kevin Wolf wrote:
> 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

which is not in qemu (at least the one you're describing)

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

Of course you're right, formally.  But that's just not a very good
argument to me, personally.  If deprecation is easy and helpful, there
is no reason to be nasty and skip it just because we are formally
allowed to do so.

Sure, the question is whether it is helpful.

> 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

OK.

> In
> addition, I pointed out how a deprecation period is useless for
> user-facing changes.

You pointed that out for users of specific distros.  I myself use Fedora
(every release) and Arch, so as a user I do see all qemu versions.

> I'm concluding that a "voluntary" deprecation for
> two releases isn't helpful at all.

Yes, you can conclude that from your reasoning.  But I disagree with
your reasoning because you are only considering users such as yourself
who skip releases on purpose or users who use long-term distros.  Most
importantly we probably have professional end users that do update qemu
every release because they know the deprecation policy, who do not use
libvirt.

I agree that adding a warning has little impact.  But I also claim that
doing it is little effort.

I mean, whatever.  I'm really not that much against taking the series as-is.

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

Of course we'd do the deprecation while fixing the bug. :-P

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

Not quite true.  What I proposed here would still allow addressing
snapshots by ID.  This series completely removes that.

I only break things for users where the tag conflicts with the ID.

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

Sure, for this case, probably.

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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