On Tue, Sep 14, 2010 at 3:35 AM, Kevin Wolf <kw...@redhat.com> wrote:
>
> 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.
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
