Am 09.03.2010 02:13, schrieb Rob Earhart:
> Modify the snapshot load path to find and load snapshots contained in backing
> disks, useful when the current disk is a differencing disk.
>
> Add the source of a snapshot when listing snapshots.
>
> This should only break backwards compatibility for scenarios depending on not
> being able to load snapshots from backing disks (which doesn't seem like a
> problem), and for code which parses the snapshot list output (if any).
>
> Signed-off-by: Rob Earhart <earh...@google.com>
I think I have considered this kind of thing some time ago, and I seem
to remember that there was something dangerous about it. However, I can
remember right away what it was. Give me a day or two to think about it
once again.
> ---
> block.c | 10 +++++
> block.h | 2 +
> block/qcow2-snapshot.c | 105
> ++++++++++++++++++++++++++++++++++++------------
> qemu-img.c | 25 +++++++-----
> savevm.c | 92 +++++++++++++++++++++++++++++-------------
> 5 files changed, 170 insertions(+), 64 deletions(-)
>
> diff --git a/block.c b/block.c
> index 31d1ba4..3588700 100644
> --- a/block.c
> +++ b/block.c
> @@ -1481,6 +1481,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
> }
> }
>
> +const char *bdrv_get_filename(BlockDriverState *bs)
> +{
> + return bs->filename;
> +}
> +
> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs)
> +{
> + return bs->backing_hd;
> +}
> +
This kind of simple getters is not the way the qemu block drivers work.
Just access bs->filename/backing_hd directly.
> int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> const uint8_t *buf, int nb_sectors)
> {
> diff --git a/block.h b/block.h
> index edf5704..1d04322 100644
> --- a/block.h
> +++ b/block.h
> @@ -179,6 +179,8 @@ int bdrv_get_info(BlockDriverState *bs,
> BlockDriverInfo *bdi);
> const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
> void bdrv_get_backing_filename(BlockDriverState *bs,
> char *filename, int filename_size);
> +const char *bdrv_get_filename(BlockDriverState *bs);
> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs);
> int bdrv_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info);
> int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 8ddaea2..ca06fcf 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -191,16 +191,23 @@ static int qcow_write_snapshots(BlockDriverState *bs)
> static void find_new_snapshot_id(BlockDriverState *bs,
> char *id_str, int id_str_size)
> {
> - BDRVQcowState *s = bs->opaque;
> - QCowSnapshot *sn;
> - int i, id, id_max = 0;
> + int id_max = 0;
> +
> + assert(bs);
> +
> + do {
> + BDRVQcowState *s = bs->opaque;
> + QCowSnapshot *sn;
> + int i, id;
> +
> + for(i = 0; i < s->nb_snapshots; i++) {
> + sn = s->snapshots + i;
> + id = strtoul(sn->id_str, NULL, 10);
> + if (id > id_max)
> + id_max = id;
> + }
> + } while ((bs = bdrv_get_backing_bdrv(bs)));
You assume that the backing file is a qcow2 file, too. This isn't
correct generally, the backing file can be any format. In that case your
code will fail horribly.
>
> - for(i = 0; i < s->nb_snapshots; i++) {
> - sn = s->snapshots + i;
> - id = strtoul(sn->id_str, NULL, 10);
> - if (id > id_max)
> - id_max = id;
> - }
> snprintf(id_str, id_str_size, "%d", id_max + 1);
> }
>
> @@ -316,41 +323,88 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
> const char *snapshot_id)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot *sn;
> - int i, snapshot_index, l1_size2;
> + int i, snapshot_index, l1_size2, ret;
>
> snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> - if (snapshot_index < 0)
> - return -ENOENT;
> +
> + if (snapshot_index < 0) {
> + /* if there is a backing file, use it. In this case, the
> + * current image needs to be reset to be consistent
> + */
> + if (bs->backing_hd) {
> + ret = bdrv_snapshot_goto(bs->backing_hd, snapshot_id);
> + if (ret < 0) {
> + return ret;
> + }
This is fundamentally broken. You can't change the backing file, and I
think bdrv_snapshot_goto does change it. It's considered read-only. This
will break all other images basing on the same backing file.
> + /* reset the sectors of the current image (Note: there can
> + * still be snapshots, so the refcounts must be kept
> + * consistent)
> + */
> + if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> + s->l1_size, -1) < 0) {
> + return -EIO;
> + }
If qemu crashes here (or the following bdrv_pwrite returns an error), we
get a corrupted image with refcounts of 0 for clusters that are still used.
> + l1_size2 = s->l1_size * sizeof(uint64_t);
> + memset(s->l1_table, 0, l1_size2);
> + if (bdrv_pwrite(s->hd, s->l1_table_offset,
> + s->l1_table, l1_size2) != l1_size2) {
> + return -EIO;
> + }
Isn't this leaking the L2 tables? And are there no in-memory data
structures that need to be invalidated, like L1/L2 caches?
If you do the thing right, I think it's an implementation for
qcow_make_empty, which if completely ifdef'd out currently. Which
probably means that doing it right is not so easy.
> + return 0;
> + } else {
> + return -ENOENT;
> + }
> + }
> +
> + assert(0 <= snapshot_index);
> +
> sn = &s->snapshots[snapshot_index];
>
> - if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> s->l1_size, -1) < 0)
> - goto fail;
> + /* if the file is read-only, all can be done by updating the
> + * in-memory L1 table. Otherwise, we'll need to do some writes.
> + */
>
> - if (qcow2_grow_l1_table(bs, sn->l1_size) < 0)
> - goto fail;
> + if (!bs->read_only) {
> + if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> + s->l1_size, -1)
> + < 0) {
> + return -EIO;
> + }
> + }
> +
> + if (qcow2_grow_l1_table(bs, sn->l1_size) < 0) {
> + return -EIO;
> + }
>
> s->l1_size = sn->l1_size;
> l1_size2 = s->l1_size * sizeof(uint64_t);
> /* copy the snapshot l1 table to the current l1 table */
> if (bdrv_pread(s->hd, sn->l1_table_offset,
> - s->l1_table, l1_size2) != l1_size2)
> - goto fail;
> - if (bdrv_pwrite(s->hd, s->l1_table_offset,
> - s->l1_table, l1_size2) != l1_size2)
> - goto fail;
> + s->l1_table, l1_size2) != l1_size2) {
> + return -EIO;
> + }
> + if (!bs->read_only) {
> + if (bdrv_pwrite(s->hd, s->l1_table_offset,
> + s->l1_table, l1_size2) != l1_size2) {
> + return -EIO;
> + }
> + }
> for(i = 0;i < s->l1_size; i++) {
> be64_to_cpus(&s->l1_table[i]);
> }
>
> - if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> s->l1_size, 1) < 0)
> - goto fail;
> + if (!bs->read_only) {
> + if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> + s->l1_size, 1)
> + < 0) {
> + return -EIO;
> + }
> + }
>
> #ifdef DEBUG_ALLOC
> qcow2_check_refcounts(bs);
> #endif
> return 0;
> - fail:
> - return -EIO;
> }
>
> int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> @@ -416,4 +470,3 @@ int qcow2_snapshot_list(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_tab)
> *psn_tab = sn_tab;
> return s->nb_snapshots;
> }
> -
> diff --git a/qemu-img.c b/qemu-img.c
> index 0c9f2d4..345a768 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -873,16 +873,21 @@ static void dump_snapshots(BlockDriverState *bs)
> int nb_sns, i;
> char buf[256];
>
> - nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> - if (nb_sns <= 0)
> - return;
> - printf("Snapshot list:\n");
> - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> - for(i = 0; i < nb_sns; i++) {
> - sn = &sn_tab[i];
> - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> - }
> - qemu_free(sn_tab);
> + assert(bs);
> +
> + do {
> + nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> + if (nb_sns <= 0)
> + continue;
> + printf("Snapshot list (from %s):\n",
> + bdrv_get_filename(bs));
> + printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + for(i = 0; i < nb_sns; i++) {
> + sn = &sn_tab[i];
> + printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> + }
> + qemu_free(sn_tab);
> + } while ((bs = bdrv_get_backing_bdrv(bs)));
IMHO, bdrv_snapshot_list should implement this and return a list of all
available snapshots.
Kevin
