From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39044 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ow9xY-0003TQ-PL for qemu-devel@nongnu.org; Thu, 16 Sep 2010 04:36:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Ow9xU-0006JM-HO for qemu-devel@nongnu.org; Thu, 16 Sep 2010 04:36:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50493) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Ow9xU-0006J4-BG for qemu-devel@nongnu.org; Thu, 16 Sep 2010 04:36:44 -0400 Message-ID: <4C91D72D.7010701@redhat.com> Date: Thu, 16 Sep 2010 10:37:01 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Copy snapshots out of QCOW2 disk References: <1284077303-2266-1-git-send-email-disheng.su@gmail.com> <4C8F4FEA.4040603@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: edison Cc: qemu-devel@nongnu.org, edison Am 15.09.2010 19:56, schrieb edison: > On Tue, Sep 14, 2010 at 3:35 AM, Kevin Wolf wrote: >> >> Am 10.09.2010 02:08, schrieb disheng.su@gmail.com: >>> diff --git a/block.h b/block.h >>> index 5f64380..9ec6219 100644 >>> --- a/block.h >>> +++ b/block.h >>> @@ -211,6 +211,8 @@ int bdrv_snapshot_goto(BlockDriverState *bs, >>> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); >>> int bdrv_snapshot_list(BlockDriverState *bs, >>> QEMUSnapshotInfo **psn_info); >>> +int bdrv_snapshot_load(BlockDriverState *bs, >>> + const char *snapshot_name); >>> char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn); >>> >>> char *get_human_readable_size(char *buf, int buf_size, int64_t size); >>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >>> index 6228612..e663e8b 100644 >>> --- a/block/qcow2-snapshot.c >>> +++ b/block/qcow2-snapshot.c >>> @@ -416,3 +416,29 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) >>> return s->nb_snapshots; >>> } >>> >>> +int qcow2_snapshot_load(BlockDriverState *bs, const char *snapshot_name) { >> >> Brace should be on a line of its own. >> >> Also, this could probably share some code with qcow2_snapshot_goto. >> >> Please add a check that the image is opened read-only. Writing to the L1 >> table after qcow2_snapshot_load() would probably have a catastrophic effect. > > Will do. > BTW, my usage case for this patch is online backup the snapshot. That > means the image is opened with r/w by a running QEMU process, at the > same time, management tool can backup some snapshots already created > before on this image. Here management tool can make sure no one take > snapshot while backup snapshots. > If I read the QCOW2 code correctly(not easy to read...), no one > snapshots_offset/nb_snapshots in QCOW2 header, if you don't take a new > snapshot, so it's OK to copy the snapshot out from the image in my > usage case, right? Right, I _think_ this might happen to work, at least with today's code. Still I discourage having an image opened twice. It's not something that code changes will take into consideration. I think the proper way to do this would be a monitor command. However, I do understand that implementing this without opening the image a second time is way harder than doing something that is targeted at working offline and just happens to work online, too. >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index a53014d..da98a01 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -1352,6 +1352,7 @@ static BlockDriver bdrv_qcow2 = { >>> .bdrv_snapshot_goto = qcow2_snapshot_goto, >>> .bdrv_snapshot_delete = qcow2_snapshot_delete, >>> .bdrv_snapshot_list = qcow2_snapshot_list, >>> + .bdrv_snapshot_load = qcow2_snapshot_load, >> >> I'm not sure if bdrv_snapshot_load is a good name. To me it sounds like >> this was the proper way to load a snapshot to continue normal work on >> it. It's much less, though. > > This is the special operation, maybe only for QCOW2 which takes > internal snapshot... > How about bdrv_snapshot_copy? Maybe just something like bdrv_snapshot_load_tmp? Kevin