Re: [Qemu-devel] [PATCH] Copy snapshots out of QCOW2 disk

[Kevin Wolf]

Am 15.09.2010 19:56, schrieb edison:
> On Tue, Sep 14, 2010 at 3:35 AM, Kevin Wolf <address@hidden> wrote:
>>
>> Am 10.09.2010 02:08, schrieb address@hidden:
>>> 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