From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60703)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <berto@igalia.com>) id 1ZbUQY-0004ei-Kx
	for qemu-devel@nongnu.org; Mon, 14 Sep 2015 10:08:15 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <berto@igalia.com>) id 1ZbUQU-0003Pf-Hh
	for qemu-devel@nongnu.org; Mon, 14 Sep 2015 10:08:14 -0400
From: Alberto Garcia <berto@igalia.com>
In-Reply-To: <55F31633.6010800@redhat.com>
References: <cover.1441890725.git.berto@igalia.com>
	<40f3f51fb44da70929f7055e95582351d1aa3f34.1441890725.git.berto@igalia.com>
	<55F31633.6010800@redhat.com>
Date: Mon, 14 Sep 2015 16:08:07 +0200
Message-ID: <w51lhc9gkl4.fsf@maestria.local.igalia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot'
	QMP command
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, qemu-block@nongnu.org, Max Reitz <mreitz@redhat.com>

On Fri 11 Sep 2015 07:58:11 PM CEST, Eric Blake wrote:

>> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
>> +                           Error **errp)
>> +{
>> +    BlockdevSnapshot snapshot_data = {
>> +        .device = (char *) device,
>> +        .snapshot = (char *) snapshot
>> +    };
>
> Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
> make this function have the saner signature of
>
> void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);
>
> rather than having to rebuild the struct yourself (with an annoying
> cast to lose const) :)
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

That's cool! We can simplify this later as you suggest.

>> +Create a snapshot, by installing 'device' as the backing image of
>> +'snapshot'. Additionally, if 'device' is associated with a block
>> +device, the block device changes to using 'snapshot' as its new
>> +active image.
>
> Still didn't answer the question from the earlier review of whether
> the blockdev-snapshot-sync behavior of specifying the node name of an
> active layer in order to not pivot the block device to the snapshot
> still makes sense to support in the blockdev-snapshot case.

Sorry if I overlooked the explanation, but I still didn't get the use
case for this.

And particularly for this command, that doesn't create any image but
simply uses a previously-opened one.

What would 'pivot': false do ? As far as I can see, specifying the node
name of an active layer in blockdev-snapshot-sync seems to pivot the
block device to the snapshot.

{ "execute": "blockdev-snapshot-sync",
             "arguments": { "node-name": "hd0",
                            "snapshot-file": "hd1.qcow2",
                            "snapshot-node-name": "hd1" } }

(qemu) info block
virtio0 (hd1): hd1.qcow2 (qcow2)
    Cache mode:       writeback
    Backing file:     hd0.qcow2 (chain depth: 1)

Berto