Am 10.09.2010 02:08, schrieb disheng...@gmail.com:
> From: edison <edi...@cloud.com>
>
> In order to backup snapshots, created from QCOW2 iamge, we want to copy
> snapshots out of QCOW2 disk to a seperate storage.
> The following patch adds a new option in "qemu-img": qemu-img convert -f
> qcow2 -O qcow2 -s snapshot_name src_img bck_img.
> Right now, it only supports to copy the full snapshot, delta snapshot is on
> the way.
>
> Signed-off-by: Disheng Su <edi...@cloud.com>
> ---
> block.c | 11 +++++++++++
> block.h | 2 ++
> block/qcow2-snapshot.c | 26 ++++++++++++++++++++++++++
> block/qcow2.c | 1 +
> block/qcow2.h | 1 +
> block_int.h | 2 ++
> qemu-img-cmds.hx | 4 ++--
> qemu-img.c | 19 ++++++++++++++++++-
> 8 files changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index ebbc376..eaf78f6 100644
> --- a/block.c
> +++ b/block.c
> @@ -1899,6 +1899,17 @@ int bdrv_snapshot_list(BlockDriverState *bs,
> return -ENOTSUP;
> }
>
> +int bdrv_snapshot_load(BlockDriverState *bs,
> + const char *snapshot_name)
> +{
> + BlockDriver *drv = bs->drv;
> + if (!drv)
> + return -ENOMEDIUM;
Please add curly braces (see CODING_STYLE for style issues)
> + if (drv->bdrv_snapshot_load)
> + return drv->bdrv_snapshot_load(bs, snapshot_name);
Indentation should be four spaces, no tabs.
> + return -ENOTSUP;
> +}
> +
> #define NB_SUFFIXES 4
>
> char *get_human_readable_size(char *buf, int buf_size, int64_t size)
> 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.
> + int i, snapshot_index, l1_size2;
> + BDRVQcowState *s = bs->opaque;
> + QCowSnapshot *sn;
> + snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name);
> + if (snapshot_index < 0)
> + return -ENOENT;
> + sn = &s->snapshots[snapshot_index];
> + s->l1_size = sn->l1_size;
> + l1_size2 = s->l1_size * sizeof(uint64_t);
> + if (s->l1_table != NULL) {
> + qemu_free(s->l1_table);
> + }
> + s->l1_table_offset = sn->l1_table_offset;
> + s->l1_table = qemu_mallocz(
> + align_offset(l1_size2, 512));
> + /* copy the snapshot l1 table to the current l1 table */
Actually, you don't copy anything but read it from the file.
> + if (bdrv_pread(bs->file, sn->l1_table_offset,
> + s->l1_table, l1_size2) != l1_size2) {
> + return -1;
> + }
> + for(i = 0;i < s->l1_size; i++) {
> + be64_to_cpus(&s->l1_table[i]);
> + }
> + return 0;
> +}
I think the whole function could use some blank lines for readability.
> 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.
> .bdrv_get_info = qcow_get_info,
>
> .bdrv_save_vmstate = qcow_save_vmstate,
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 3ff162e..cbbba48 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -211,6 +211,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info);
> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
> +int qcow2_snapshot_load(BlockDriverState *bs, const char *snapshot_name);
>
> void qcow2_free_snapshots(BlockDriverState *bs);
> int qcow2_read_snapshots(BlockDriverState *bs);
> diff --git a/block_int.h b/block_int.h
> index e8e7156..93d5a1b 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -93,6 +93,8 @@ struct BlockDriver {
> 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);
> int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>
> int (*bdrv_save_vmstate)(BlockDriverState *bs, const uint8_t *buf,
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 6d3e5f8..6c7176f 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -28,9 +28,9 @@ STEXI
> ETEXI
>
> DEF("convert", img_convert,
> - "convert [-c] [-f fmt] [-O output_fmt] [-o options] filename [filename2
> [...]] output_filename")
> + "convert [-c] [-f fmt] [-O output_fmt] [-o options] [-s snapshot_name]
> filename [filename2 [...]] output_filename")
> STEXI
> -...@item convert [-c] [-f @var{fmt}] [-O @var{output_fmt}] [-o
> @var{options}] @var{filename} [...@var{filename2} [...]] @var{output_filename}
> +...@item convert [-c] [-f @var{fmt}] [-O @var{output_fmt}] [-o
> @var{options}] [-s @var{snapshot_name}] @var{filename} [...@var{filename2}
> [...]] @var{output_filename}
> ETEXI
>
> DEF("info", img_info,
Please don't forget to add documentation to qemu-img.texi, too.
> diff --git a/qemu-img.c b/qemu-img.c
> index 4e035e4..7ff015d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -646,13 +646,14 @@ static int img_convert(int argc, char **argv)
> BlockDriverInfo bdi;
> QEMUOptionParameter *param = NULL, *create_options = NULL;
> char *options = NULL;
> + const char *snapshot_name = NULL;
>
> fmt = NULL;
> out_fmt = "raw";
> out_baseimg = NULL;
> flags = 0;
> for(;;) {
> - c = getopt(argc, argv, "f:O:B:hce6o:");
> + c = getopt(argc, argv, "f:O:B:s:hce6o:");
> if (c == -1)
> break;
> switch(c) {
> @@ -680,6 +681,9 @@ static int img_convert(int argc, char **argv)
> case 'o':
> options = optarg;
> break;
> + case 's':
> + snapshot_name = optarg;
> + break;
> }
> }
>
> @@ -711,6 +715,19 @@ static int img_convert(int argc, char **argv)
> total_sectors += bs_sectors;
> }
>
> + if (snapshot_name != NULL) {
> + if (bs_n > 1) {
> + error("No support for concatenating multiple snapshot\n");
> + ret = -1;
> + goto out;
> + }
> + if (bdrv_snapshot_load(bs[0], snapshot_name) < 0) {
> + error("Failed to load snapshot\n");
> + ret = -1;
> + goto out;
> + }
> + }
Something's wrong with the indentation in this hunk.
Kevin
