| Message ID | 377f3a9b1003081713v6f39fae2sb15f80073d13215a@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 <earhart@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
On 03/08/2010 07:13 PM, Rob Earhart wrote: > 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<earhart@google.com> > Your patch is whitespace damaged. Please use a different mailer (like git-send-email). Regards, Anthony Liguori > --- > 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; > +} > + > 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))); > > - 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; > + } > + /* 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; > + } > + 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; > + } > + 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))); > } > > static int img_info(int argc, char **argv) > diff --git a/savevm.c b/savevm.c > index 2fd3de6..8a81c42 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1616,25 +1616,53 @@ static BlockDriverState *get_bs_snapshots(void) > return bs; > } > > +/* Find a snapshot with the given name. > + * sn_bs is an out parameter: if NULL, only the current BlockDriverState (bs) > + * is checked; otherwise, *sb_bs is set to point to the backing block driver > + * state where the found snapshot resides. > + */ > static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > - const char *name) > + const char *name, BlockDriverState **sn_bs) > { > QEMUSnapshotInfo *sn_tab, *sn; > int nb_sns, i, ret; > > + assert(bs); > + > ret = -ENOENT; > - nb_sns = bdrv_snapshot_list(bs,&sn_tab); > - if (nb_sns< 0) > - return ret; > - for(i = 0; i< nb_sns; i++) { > - sn =&sn_tab[i]; > - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > - *sn_info = *sn; > - ret = 0; > + do { > + nb_sns = bdrv_snapshot_list(bs,&sn_tab); > + if (nb_sns< 0) { > + if (nb_sns == -ENOTSUP) { > + /* If this device does not support snapshots, > + * its backing file still might; continue on > + * to the backing file. > + */ > + continue; > + } else { > + return ret; > + } > + } > + for (i = 0; i< nb_sns; i++) { > + sn =&sn_tab[i]; > + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > + *sn_info = *sn; > + if (sn_bs != 0) { > + *sn_bs = bs; > + } > + ret = 0; > + break; > + } > + } > + qemu_free(sn_tab); > + if (ret == 0) { > break; > } > - } > - qemu_free(sn_tab); > + if (!sn_bs) { > + break; > + } > + } while ((bs = bdrv_get_backing_bdrv(bs))); > + > return ret; > } > > @@ -1651,7 +1679,7 @@ static int del_existing_snapshots(Monitor *mon, > const char *name) > QTAILQ_FOREACH(dinfo,&drives, next) { > bs = dinfo->bdrv; > if (bdrv_can_snapshot(bs)&& > - bdrv_snapshot_find(bs, snapshot, name)>= 0) > + bdrv_snapshot_find(bs, snapshot, name, NULL)>= 0) > { > ret = bdrv_snapshot_delete(bs, name); > if (ret< 0) { > @@ -1696,7 +1724,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) > > memset(sn, 0, sizeof(*sn)); > if (name) { > - ret = bdrv_snapshot_find(bs, old_sn, name); > + ret = bdrv_snapshot_find(bs, old_sn, name, NULL); > if (ret>= 0) { > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > @@ -1759,7 +1787,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) > int load_vmstate(Monitor *mon, const char *name) > { > DriveInfo *dinfo; > - BlockDriverState *bs, *bs1; > + BlockDriverState *bs, *bs1, *sn_bs; > QEMUSnapshotInfo sn; > QEMUFile *f; > int ret; > @@ -1804,12 +1832,12 @@ int load_vmstate(Monitor *mon, const char *name) > } > > /* Don't even try to load empty VM states */ > - ret = bdrv_snapshot_find(bs,&sn, name); > + ret = bdrv_snapshot_find(bs,&sn, name,&sn_bs); > if ((ret>= 0)&& (sn.vm_state_size == 0)) > return -EINVAL; > > /* restore the VM state */ > - f = qemu_fopen_bdrv(bs, 0); > + f = qemu_fopen_bdrv(sn_bs, 0); > if (!f) { > monitor_printf(mon, "Could not open VM state file\n"); > return -EINVAL; > @@ -1876,17 +1904,25 @@ void do_info_snapshots(Monitor *mon) > } > monitor_printf(mon, "\n"); > > - nb_sns = bdrv_snapshot_list(bs,&sn_tab); > - if (nb_sns< 0) { > - monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns); > - return; > - } > - monitor_printf(mon, "Snapshot list (from %s):\n", > - bdrv_get_device_name(bs)); > - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); > - for(i = 0; i< nb_sns; i++) { > - sn =&sn_tab[i]; > - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); > + for (; bs; bs = bdrv_get_backing_bdrv(bs)) { > + nb_sns = bdrv_snapshot_list(bs,&sn_tab); > + if (nb_sns< 0) { > + monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns); > + continue; > + } > + const char* device_name = bdrv_get_device_name(bs); > + if (device_name&& device_name[0]) { > + monitor_printf(mon, "Snapshot list (device %s, from %s):\n", > + device_name, bdrv_get_filename(bs)); > + } else { > + monitor_printf(mon, "Snapshot list (from %s):\n", > + bdrv_get_filename(bs)); > + } > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, > sizeof(buf), NULL)); > + for(i = 0; i< nb_sns; i++) { > + sn =&sn_tab[i]; > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, > sizeof(buf), sn)); > + } > + qemu_free(sn_tab); > } > - qemu_free(sn_tab); > } >
On Wed, Mar 17, 2010 at 8:35 AM, Anthony Liguori <anthony@codemonkey.ws>wrote: > On 03/08/2010 07:13 PM, Rob Earhart wrote: > >> 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<earhart@google.com> >> >> > > Your patch is whitespace damaged. Please use a different mailer (like > git-send-email). > > Regards, > > Anthony Liguori > > If git-send-email worked in my environment, I'd be happy to. :-) I'll work on it. I still need to address Kevin's comments from last week before I worry about that, though. )Rob > > --- >> 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; >> +} >> + >> 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))); >> >> - 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; >> + } >> + /* 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; >> + } >> + 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; >> + } >> + 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))); >> } >> >> static int img_info(int argc, char **argv) >> diff --git a/savevm.c b/savevm.c >> index 2fd3de6..8a81c42 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -1616,25 +1616,53 @@ static BlockDriverState *get_bs_snapshots(void) >> return bs; >> } >> >> +/* Find a snapshot with the given name. >> + * sn_bs is an out parameter: if NULL, only the current BlockDriverState >> (bs) >> + * is checked; otherwise, *sb_bs is set to point to the backing block >> driver >> + * state where the found snapshot resides. >> + */ >> static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo >> *sn_info, >> - const char *name) >> + const char *name, BlockDriverState **sn_bs) >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> int nb_sns, i, ret; >> >> + assert(bs); >> + >> ret = -ENOENT; >> - nb_sns = bdrv_snapshot_list(bs,&sn_tab); >> - if (nb_sns< 0) >> - return ret; >> - for(i = 0; i< nb_sns; i++) { >> - sn =&sn_tab[i]; >> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> - *sn_info = *sn; >> - ret = 0; >> + do { >> + nb_sns = bdrv_snapshot_list(bs,&sn_tab); >> + if (nb_sns< 0) { >> + if (nb_sns == -ENOTSUP) { >> + /* If this device does not support snapshots, >> + * its backing file still might; continue on >> + * to the backing file. >> + */ >> + continue; >> + } else { >> + return ret; >> + } >> + } >> + for (i = 0; i< nb_sns; i++) { >> + sn =&sn_tab[i]; >> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + if (sn_bs != 0) { >> + *sn_bs = bs; >> + } >> + ret = 0; >> + break; >> + } >> + } >> + qemu_free(sn_tab); >> + if (ret == 0) { >> break; >> } >> - } >> - qemu_free(sn_tab); >> + if (!sn_bs) { >> + break; >> + } >> + } while ((bs = bdrv_get_backing_bdrv(bs))); >> + >> return ret; >> } >> >> @@ -1651,7 +1679,7 @@ static int del_existing_snapshots(Monitor *mon, >> const char *name) >> QTAILQ_FOREACH(dinfo,&drives, next) { >> bs = dinfo->bdrv; >> if (bdrv_can_snapshot(bs)&& >> - bdrv_snapshot_find(bs, snapshot, name)>= 0) >> + bdrv_snapshot_find(bs, snapshot, name, NULL)>= 0) >> { >> ret = bdrv_snapshot_delete(bs, name); >> if (ret< 0) { >> @@ -1696,7 +1724,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> >> memset(sn, 0, sizeof(*sn)); >> if (name) { >> - ret = bdrv_snapshot_find(bs, old_sn, name); >> + ret = bdrv_snapshot_find(bs, old_sn, name, NULL); >> if (ret>= 0) { >> pstrcpy(sn->name, sizeof(sn->name), old_sn->name); >> pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); >> @@ -1759,7 +1787,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> int load_vmstate(Monitor *mon, const char *name) >> { >> DriveInfo *dinfo; >> - BlockDriverState *bs, *bs1; >> + BlockDriverState *bs, *bs1, *sn_bs; >> QEMUSnapshotInfo sn; >> QEMUFile *f; >> int ret; >> @@ -1804,12 +1832,12 @@ int load_vmstate(Monitor *mon, const char *name) >> } >> >> /* Don't even try to load empty VM states */ >> - ret = bdrv_snapshot_find(bs,&sn, name); >> + ret = bdrv_snapshot_find(bs,&sn, name,&sn_bs); >> if ((ret>= 0)&& (sn.vm_state_size == 0)) >> return -EINVAL; >> >> /* restore the VM state */ >> - f = qemu_fopen_bdrv(bs, 0); >> + f = qemu_fopen_bdrv(sn_bs, 0); >> if (!f) { >> monitor_printf(mon, "Could not open VM state file\n"); >> return -EINVAL; >> @@ -1876,17 +1904,25 @@ void do_info_snapshots(Monitor *mon) >> } >> monitor_printf(mon, "\n"); >> >> - nb_sns = bdrv_snapshot_list(bs,&sn_tab); >> - if (nb_sns< 0) { >> - monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns); >> - return; >> - } >> - monitor_printf(mon, "Snapshot list (from %s):\n", >> - bdrv_get_device_name(bs)); >> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), >> NULL)); >> - for(i = 0; i< nb_sns; i++) { >> - sn =&sn_tab[i]; >> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), >> sn)); >> + for (; bs; bs = bdrv_get_backing_bdrv(bs)) { >> + nb_sns = bdrv_snapshot_list(bs,&sn_tab); >> + if (nb_sns< 0) { >> + monitor_printf(mon, "bdrv_snapshot_list: error %d\n", >> nb_sns); >> + continue; >> + } >> + const char* device_name = bdrv_get_device_name(bs); >> + if (device_name&& device_name[0]) { >> + monitor_printf(mon, "Snapshot list (device %s, from %s):\n", >> + device_name, bdrv_get_filename(bs)); >> + } else { >> + monitor_printf(mon, "Snapshot list (from %s):\n", >> + bdrv_get_filename(bs)); >> + } >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, >> sizeof(buf), NULL)); >> + for(i = 0; i< nb_sns; i++) { >> + sn =&sn_tab[i]; >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, >> sizeof(buf), sn)); >> + } >> + qemu_free(sn_tab); >> } >> - qemu_free(sn_tab); >> } >> >> > >
Am 11.03.2010 14:12, schrieb Kevin Wolf: > 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 <earhart@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. One thing that came to my mind is the following: 1. Load a snapshot from the backing file 2. Run the VM for a while 3. savevm (to the COW file) 4. Load a different snapshot from the backing file 5. Load the snapshot saved in 3. At this point we need not only load the snapshot in the COW file, but we also need to load the backing file snapshot on which this snapshot is based. Therefore we need to save the name of the current snapshot in the whole backing file chain. This needs a file format change. Luckily (and surprisingly) the snapshot header seems to be about the only place in qcow2 which is actually prepared for extensions - at least it has an extra_data_size field. So I think it might be possible to implement it in a backwards compatible way at least (however, don't try to open such images in older qemu versions then, they would fail horribly). Kevin
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; +} + 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))); - 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; + } + /* 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; + } + 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; + } + 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))); } static int img_info(int argc, char **argv) diff --git a/savevm.c b/savevm.c index 2fd3de6..8a81c42 100644 --- a/savevm.c +++ b/savevm.c @@ -1616,25 +1616,53 @@ static BlockDriverState *get_bs_snapshots(void) return bs; } +/* Find a snapshot with the given name. + * sn_bs is an out parameter: if NULL, only the current BlockDriverState (bs) + * is checked; otherwise, *sb_bs is set to point to the backing block driver + * state where the found snapshot resides. + */ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, - const char *name) + const char *name, BlockDriverState **sn_bs) { QEMUSnapshotInfo *sn_tab, *sn; int nb_sns, i, ret; + assert(bs); + ret = -ENOENT; - nb_sns = bdrv_snapshot_list(bs, &sn_tab); - if (nb_sns < 0) - return ret; - for(i = 0; i < nb_sns; i++) { - sn = &sn_tab[i]; - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { - *sn_info = *sn; - ret = 0; + do { + nb_sns = bdrv_snapshot_list(bs, &sn_tab); + if (nb_sns < 0) { + if (nb_sns == -ENOTSUP) { + /* If this device does not support snapshots, + * its backing file still might; continue on + * to the backing file. + */ + continue; + } else { + return ret; + } + } + for (i = 0; i < nb_sns; i++) { + sn = &sn_tab[i]; + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { + *sn_info = *sn; + if (sn_bs != 0) { + *sn_bs = bs; + } + ret = 0; + break; + } + } + qemu_free(sn_tab); + if (ret == 0) { break; } - } - qemu_free(sn_tab); + if (!sn_bs) { + break; + } + } while ((bs = bdrv_get_backing_bdrv(bs))); + return ret; } @@ -1651,7 +1679,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) QTAILQ_FOREACH(dinfo, &drives, next) { bs = dinfo->bdrv; if (bdrv_can_snapshot(bs) && - bdrv_snapshot_find(bs, snapshot, name) >= 0) + bdrv_snapshot_find(bs, snapshot, name, NULL) >= 0) { ret = bdrv_snapshot_delete(bs, name);
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 <earhart@google.com> --- 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(-) if (ret < 0) { @@ -1696,7 +1724,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) memset(sn, 0, sizeof(*sn)); if (name) { - ret = bdrv_snapshot_find(bs, old_sn, name); + ret = bdrv_snapshot_find(bs, old_sn, name, NULL); if (ret >= 0) { pstrcpy(sn->name, sizeof(sn->name), old_sn->name); pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); @@ -1759,7 +1787,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) int load_vmstate(Monitor *mon, const char *name) { DriveInfo *dinfo; - BlockDriverState *bs, *bs1; + BlockDriverState *bs, *bs1, *sn_bs; QEMUSnapshotInfo sn; QEMUFile *f; int ret; @@ -1804,12 +1832,12 @@ int load_vmstate(Monitor *mon, const char *name) } /* Don't even try to load empty VM states */ - ret = bdrv_snapshot_find(bs, &sn, name); + ret = bdrv_snapshot_find(bs, &sn, name, &sn_bs); if ((ret >= 0) && (sn.vm_state_size == 0)) return -EINVAL; /* restore the VM state */ - f = qemu_fopen_bdrv(bs, 0); + f = qemu_fopen_bdrv(sn_bs, 0); if (!f) { monitor_printf(mon, "Could not open VM state file\n"); return -EINVAL; @@ -1876,17 +1904,25 @@ void do_info_snapshots(Monitor *mon) } monitor_printf(mon, "\n"); - nb_sns = bdrv_snapshot_list(bs, &sn_tab); - if (nb_sns < 0) { - monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns); - return; - } - monitor_printf(mon, "Snapshot list (from %s):\n", - bdrv_get_device_name(bs)); - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); - for(i = 0; i < nb_sns; i++) { - sn = &sn_tab[i]; - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); + for (; bs; bs = bdrv_get_backing_bdrv(bs)) { + nb_sns = bdrv_snapshot_list(bs, &sn_tab); + if (nb_sns < 0) { + monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns); + continue; + } + const char* device_name = bdrv_get_device_name(bs); + if (device_name && device_name[0]) { + monitor_printf(mon, "Snapshot list (device %s, from %s):\n", + device_name, bdrv_get_filename(bs)); + } else { + monitor_printf(mon, "Snapshot list (from %s):\n", + bdrv_get_filename(bs)); + } + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); + for(i = 0; i < nb_sns; i++) { + sn = &sn_tab[i]; + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); + } + qemu_free(sn_tab); } - qemu_free(sn_tab); }