[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: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore |
Date: |
Thu, 10 Jan 2019 17:06:27 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
* Kevin Wolf (address@hidden) wrote:
> Am 10.01.2019 um 12:41 hat Dr. David Alan Gilbert geschrieben:
> > * 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?
> > >
> > > > 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]
> >
> > If we made that:
> >
> > savevm [-t] [-i] [tag|id]
> >
> > then:
> > a) with neither -t or -i it would behave in the same roulette way
> > as it does in the moment, and it might be a tag or id
> >
> > b) with -t we'd explicitly treat the parameter as a tag and it
> > would error if it wasn't found
> >
> > c) With -i we'd explicitly treat the parameter as an id and
> > it would error if it wasn't found
> >
> > Since we still allow (a) it doesn't break any existing code.
>
> If you can explain why we need both tag and id?
>
> And by keeping the current behaviour, we might not break hypothetically
> existing correct code, but we leave currently actually existing broken
> code like libvirt broken.
My only reason for leaving both tag & id was for the hypothetical
existing current code; my assumption adding the above would be that we
would then fix libvirt never to use (a), probably always (b).
Dave
> Kevin
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, (continued)
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Eric Blake, 2019/01/09
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Kevin Wolf, 2019/01/09
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Eric Blake, 2019/01/09
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Kevin Wolf, 2019/01/10
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Dr. David Alan Gilbert, 2019/01/10
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Daniel Henrique Barboza, 2019/01/10
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Kevin Wolf, 2019/01/10
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Eric Blake, 2019/01/10
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Kevin Wolf, 2019/01/11
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Max Reitz, 2019/01/11
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Kevin Wolf, 2019/01/11
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Max Reitz, 2019/01/11
- Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Daniel Henrique Barboza, 2019/01/09
Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore, Eric Blake, 2019/01/09