Patchwork Copy snapshots out of QCOW2 disk

login
register
mail settings
Submitter disheng.su@gmail.com
Date Sept. 10, 2010, 12:08 a.m.
Message ID <1284077303-2266-1-git-send-email-disheng.su@gmail.com>
Download mbox | patch
Permalink /patch/64341/
State New
Headers show

Comments

disheng.su@gmail.com - Sept. 10, 2010, 12:08 a.m.
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(-)
Kevin Wolf - Sept. 14, 2010, 10:35 a.m.
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
disheng.su@gmail.com - Sept. 15, 2010, 5:56 p.m.
On Tue, Sep 14, 2010 at 3:35 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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.

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?

>
> > +    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.

I see, this is the copy-paste from qcow2_snapshot_goto...:)

>
> > +    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.

Will do.

>
> > 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?

>
> >      .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.

Will do.

>
> > 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.

I see, will change it to 4 space.

>
> Kevin
Kevin Wolf - Sept. 16, 2010, 8:37 a.m.
Am 15.09.2010 19:56, schrieb edison:
> On Tue, Sep 14, 2010 at 3:35 AM, Kevin Wolf <kwolf@redhat.com> 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

Patch

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;
+    if (drv->bdrv_snapshot_load)
+	return drv->bdrv_snapshot_load(bs, snapshot_name);
+    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) {
+    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 */
+    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;
+}
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,
     .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,
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;
+	}
+    }
+
     /* Find driver and parse its options */
     drv = bdrv_find_format(out_fmt);
     if (!drv) {