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

[Kevin Wolf <kwolf@redhat.com>]

Am 10.09.2010 02:08, schrieb disheng.su@gmail.com:
> From: edison <edison@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 <edison@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