All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>,
	armbru@redhat.com,
	Daniel Henrique Barboza <danielhb413@gmail.com>,
	qemu-devel@nongnu.org, muriloo@linux.ibm.com,
	dgilbert@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore
Date: Thu, 10 Jan 2019 12:25:08 +0100	[thread overview]
Message-ID: <20190110112508.GA6361@linux.fritz.box> (raw)
In-Reply-To: <56880d06-a214-2486-2e80-c565b33461b3@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]

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?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

  reply	other threads:[~2019-01-10 11:25 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-06 11:11 [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore Daniel Henrique Barboza
2018-09-06 11:11 ` [Qemu-devel] [PATCH v2 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations Daniel Henrique Barboza
2018-09-06 11:11 ` [Qemu-devel] [PATCH v2 2/3] block/snapshot: remove bdrv_snapshot_delete_by_id_or_name Daniel Henrique Barboza
2018-09-06 11:11 ` [Qemu-devel] [PATCH v2 3/3] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call Daniel Henrique Barboza
2018-10-08 18:13 ` [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore Daniel Henrique Barboza
     [not found] ` <20180921122954.GD2842@work-vm>
     [not found]   ` <355a1147-c0d0-88fc-7b68-4391bab25c54@gmail.com>
2018-10-09 17:34     ` Markus Armbruster
2018-10-10  7:56       ` [Qemu-devel] [libvirt] " Peter Krempa
2018-10-11 12:06         ` Dr. David Alan Gilbert
2018-10-11 12:35           ` Peter Krempa
2019-01-09 14:10 ` [Qemu-devel] " Max Reitz
2019-01-09 14:21   ` Kevin Wolf
2019-01-09 14:27     ` Max Reitz
2019-01-09 14:48       ` Kevin Wolf
2019-01-09 14:54         ` Max Reitz
2019-01-09 15:13           ` Kevin Wolf
2019-01-09 15:25             ` Dr. David Alan Gilbert
2019-01-09 16:25             ` Markus Armbruster
2019-01-09 16:27             ` Max Reitz
2019-01-09 16:45               ` Kevin Wolf
2019-01-09 16:58                 ` Max Reitz
2019-01-09 18:19     ` Daniel Henrique Barboza
2019-01-09 16:57   ` Daniel Henrique Barboza
2019-01-09 17:05     ` Max Reitz
2019-01-09 17:20       ` Kevin Wolf
2019-01-09 17:38         ` Max Reitz
2019-01-09 17:52           ` Eric Blake
2019-01-09 19:00             ` Kevin Wolf
2019-01-11 12:35             ` Max Reitz
2019-01-09 17:55           ` Eric Blake
2019-01-09 18:51             ` Kevin Wolf
2019-01-09 19:02               ` Eric Blake
2019-01-10 11:25                 ` Kevin Wolf [this message]
2019-01-10 11:41                   ` Dr. David Alan Gilbert
2019-01-10 13:03                     ` Daniel Henrique Barboza
2019-01-10 15:11                     ` Kevin Wolf
2019-01-10 17:06                       ` Dr. David Alan Gilbert
2019-01-10 18:22                         ` Eric Blake
2019-01-11 12:14                           ` Kevin Wolf
2019-01-11 13:22                     ` Max Reitz
2019-01-11 14:33                       ` Kevin Wolf
2019-01-11 15:23                         ` Max Reitz
2019-01-09 17:32       ` Daniel Henrique Barboza
2019-01-09 17:07     ` Eric Blake

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190110112508.GA6361@linux.fritz.box \
    --to=kwolf@redhat.com \
    --cc=armbru@redhat.com \
    --cc=danielhb413@gmail.com \
    --cc=dgilbert@redhat.com \
    --cc=eblake@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=muriloo@linux.ibm.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.