From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=33914 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OCuQy-00013H-CX
	for qemu-devel@nongnu.org; Fri, 14 May 2010 08:56:09 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1OCuQw-00074b-6s
	for qemu-devel@nongnu.org; Fri, 14 May 2010 08:56:08 -0400
Received: from mx1.redhat.com ([209.132.183.28]:22375)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1OCuQv-00074R-Vp
	for qemu-devel@nongnu.org; Fri, 14 May 2010 08:56:06 -0400
Message-ID: <4BED4827.5010709@redhat.com>
Date: Fri, 14 May 2010 14:55:03 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <1273661213-19611-1-git-send-email-morita.kazutaka@lab.ntt.co.jp>
	<1273830676-2349-3-git-send-email-morita.kazutaka@lab.ntt.co.jp>
In-Reply-To: <1273830676-2349-3-git-send-email-morita.kazutaka@lab.ntt.co.jp>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [RFC PATCH v2 2/3] block: call the snapshot
 handlers of the protocol drivers
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/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: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: aliguori@us.ibm.com, sheepdog@lists.wpkg.org, kvm@vger.kernel.org, qemu-devel@nongnu.org, fujita.tomonori@lab.ntt.co.jp, avi@redhat.com, hch@lst.de

Am 14.05.2010 11:51, schrieb MORITA Kazutaka:
> When snapshot handlers of the format driver is not defined, it is
> better to call the ones of the protocol driver.
> 
> This enables us to implement snapshot support in the protocol driver.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

>  int bdrv_snapshot_goto(BlockDriverState *bs,
> @@ -1737,9 +1743,11 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      BlockDriver *drv = bs->drv;
>      if (!drv)
>          return -ENOMEDIUM;
> -    if (!drv->bdrv_snapshot_goto)
> -        return -ENOTSUP;
> -    return drv->bdrv_snapshot_goto(bs, snapshot_id);
> +    if (drv->bdrv_snapshot_goto)
> +        return drv->bdrv_snapshot_goto(bs, snapshot_id);
> +    if (bs->file)
> +        return bdrv_snapshot_goto(bs->file, snapshot_id);
> +    return -ENOTSUP;
>  }

During lunch one more thing came to my mind...

For bdrv_snapshot_goto it's probably not that easy for formats other
than raw. Usually the drivers keep some state in memory, and with this
code they won't re-read it after the snapshot has been loaded - which
will lead to an inconsistent state very easily.

I think the right thing to do here is:

1. Call drv->bdrv_close() so that the format driver cleans up, but the
underlying protocol stays open
2. Load the snapshot on the protocol level
3. Call drv->bdrv_open() to load the new state

The other functions in this patch should be okay without re-opening
because they don't change the current state.

Kevin