| Message ID | 1277484812-22012-10-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Of course specifying an explicit medium for snapshot, be that the
snapshot section of a qcow2 image or just a separate flat file and
managing that one explicitly would be even better.
Christoph Hellwig <hch@lst.de> writes: > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Of course specifying an explicit medium for snapshot, be that the > snapshot section of a qcow2 image or just a separate flat file and > managing that one explicitly would be even better. Indeed.
Am 25.06.2010 18:53, schrieb Markus Armbruster: > savevm.c keeps a pointer to the snapshot block device. If you manage > to get that device deleted, the pointer dangles, and the next snapshot > operation will crash & burn. Unplugging a guest device that uses it > does the trick: > > $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...] > QEMU 0.12.50 monitor - type 'help' for more information > (qemu) info snapshots > No available block device supports snapshots > (qemu) drive_add auto if=none,file=tmp.qcow2 > OK > (qemu) device_add usb-storage,id=foo,drive=none1 > (qemu) info snapshots > Snapshot devices: none1 > Snapshot list (from none1): > ID TAG VM SIZE DATE VM CLOCK > (qemu) device_del foo > (qemu) info snapshots > Snapshot devices: > Segmentation fault (core dumped) > > Move management of that pointer to block.c, and zap it when the device > it points to goes away. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 25 +++++++++++++++++++++++++ > block.h | 1 + > savevm.c | 31 ++++--------------------------- > 3 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/block.c b/block.c > index 5e0ffa0..34055e0 100644 > --- a/block.c > +++ b/block.c > @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states = > static QLIST_HEAD(, BlockDriver) bdrv_drivers = > QLIST_HEAD_INITIALIZER(bdrv_drivers); > > +/* The device to use for VM snapshots */ > +static BlockDriverState *bs_snapshots; > + > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > > @@ -660,6 +663,9 @@ void bdrv_close_all(void) > void bdrv_delete(BlockDriverState *bs) > { > assert(!bs->peer); > + if (bs == bs_snapshots) { > + bs_snapshots = NULL; > + } This should probably be in bdrv_close() instead. A BlockDriverState can be closed, but not deleted yet; it can't handle snapshots in this state, though. > /* remove from list, if necessary */ > if (bs->device_name[0] != '\0') { > @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs) > return 1; > } > > +BlockDriverState *bdrv_snapshots(void) > +{ > + BlockDriverState *bs; > + > + if (bs_snapshots) > + return bs_snapshots; I know that this function is just moved with no changes, but while we're at it and you need to respin anyway, can we add braces here? > + > + bs = NULL; > + while ((bs = bdrv_next(bs))) { > + if (bdrv_can_snapshot(bs)) { > + goto ok; > + } > + } > + return NULL; > + ok: > + bs_snapshots = bs; > + return bs; > +} And instead of a goto we could do the right thing directly in the if block. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 25.06.2010 18:53, schrieb Markus Armbruster: >> savevm.c keeps a pointer to the snapshot block device. If you manage >> to get that device deleted, the pointer dangles, and the next snapshot >> operation will crash & burn. Unplugging a guest device that uses it >> does the trick: >> >> $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...] >> QEMU 0.12.50 monitor - type 'help' for more information >> (qemu) info snapshots >> No available block device supports snapshots >> (qemu) drive_add auto if=none,file=tmp.qcow2 >> OK >> (qemu) device_add usb-storage,id=foo,drive=none1 >> (qemu) info snapshots >> Snapshot devices: none1 >> Snapshot list (from none1): >> ID TAG VM SIZE DATE VM CLOCK >> (qemu) device_del foo >> (qemu) info snapshots >> Snapshot devices: >> Segmentation fault (core dumped) >> >> Move management of that pointer to block.c, and zap it when the device >> it points to goes away. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block.c | 25 +++++++++++++++++++++++++ >> block.h | 1 + >> savevm.c | 31 ++++--------------------------- >> 3 files changed, 30 insertions(+), 27 deletions(-) >> >> diff --git a/block.c b/block.c >> index 5e0ffa0..34055e0 100644 >> --- a/block.c >> +++ b/block.c >> @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states = >> static QLIST_HEAD(, BlockDriver) bdrv_drivers = >> QLIST_HEAD_INITIALIZER(bdrv_drivers); >> >> +/* The device to use for VM snapshots */ >> +static BlockDriverState *bs_snapshots; >> + >> /* If non-zero, use only whitelisted block drivers */ >> static int use_bdrv_whitelist; >> >> @@ -660,6 +663,9 @@ void bdrv_close_all(void) >> void bdrv_delete(BlockDriverState *bs) >> { >> assert(!bs->peer); >> + if (bs == bs_snapshots) { >> + bs_snapshots = NULL; >> + } > > This should probably be in bdrv_close() instead. A BlockDriverState can > be closed, but not deleted yet; it can't handle snapshots in this state, > though. Right. I was thinking about the dangling pointer only. My patch works as advertized: it fixes the crash. But zapping bs_snapshots in bdrv_close() is even better. I'll respin. >> /* remove from list, if necessary */ >> if (bs->device_name[0] != '\0') { >> @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs) >> return 1; >> } >> >> +BlockDriverState *bdrv_snapshots(void) >> +{ >> + BlockDriverState *bs; >> + >> + if (bs_snapshots) >> + return bs_snapshots; > > I know that this function is just moved with no changes, but while we're > at it and you need to respin anyway, can we add braces here? > >> + >> + bs = NULL; >> + while ((bs = bdrv_next(bs))) { >> + if (bdrv_can_snapshot(bs)) { >> + goto ok; >> + } >> + } >> + return NULL; >> + ok: >> + bs_snapshots = bs; >> + return bs; >> +} > > And instead of a goto we could do the right thing directly in the if block. Separate patch. I hate it when people squash code motion and change together.
diff --git a/block.c b/block.c index 5e0ffa0..34055e0 100644 --- a/block.c +++ b/block.c @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states = static QLIST_HEAD(, BlockDriver) bdrv_drivers = QLIST_HEAD_INITIALIZER(bdrv_drivers); +/* The device to use for VM snapshots */ +static BlockDriverState *bs_snapshots; + /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -660,6 +663,9 @@ void bdrv_close_all(void) void bdrv_delete(BlockDriverState *bs) { assert(!bs->peer); + if (bs == bs_snapshots) { + bs_snapshots = NULL; + } /* remove from list, if necessary */ if (bs->device_name[0] != '\0') { @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs) return 1; } +BlockDriverState *bdrv_snapshots(void) +{ + BlockDriverState *bs; + + if (bs_snapshots) + return bs_snapshots; + + bs = NULL; + while ((bs = bdrv_next(bs))) { + if (bdrv_can_snapshot(bs)) { + goto ok; + } + } + return NULL; + ok: + bs_snapshots = bs; + return bs; +} + int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) { diff --git a/block.h b/block.h index 88ac06e..012c2a1 100644 --- a/block.h +++ b/block.h @@ -193,6 +193,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); int bdrv_can_snapshot(BlockDriverState *bs); +BlockDriverState *bdrv_snapshots(void); int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, diff --git a/savevm.c b/savevm.c index 20354a8..f1f450e 100644 --- a/savevm.c +++ b/savevm.c @@ -83,9 +83,6 @@ #include "qemu_socket.h" #include "qemu-queue.h" -/* point to the block driver where the snapshots are managed */ -static BlockDriverState *bs_snapshots; - #define SELF_ANNOUNCE_ROUNDS 5 #ifndef ETH_P_RARP @@ -1575,26 +1572,6 @@ out: return ret; } -static BlockDriverState *get_bs_snapshots(void) -{ - BlockDriverState *bs; - - if (bs_snapshots) - return bs_snapshots; - /* FIXME what if bs_snapshots gets hot-unplugged? */ - - bs = NULL; - while ((bs = bdrv_next(bs))) { - if (bdrv_can_snapshot(bs)) { - goto ok; - } - } - return NULL; - ok: - bs_snapshots = bs; - return bs; -} - static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *name) { @@ -1674,7 +1651,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) } } - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No block device can accept snapshots\n"); return; @@ -1769,7 +1746,7 @@ int load_vmstate(const char *name) } } - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { error_report("No block device supports snapshots"); return -EINVAL; @@ -1833,7 +1810,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) int ret; const char *name = qdict_get_str(qdict, "name"); - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No block device supports snapshots\n"); return; @@ -1863,7 +1840,7 @@ void do_info_snapshots(Monitor *mon) int nb_sns, i; char buf[256]; - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); return;
savevm.c keeps a pointer to the snapshot block device. If you manage to get that device deleted, the pointer dangles, and the next snapshot operation will crash & burn. Unplugging a guest device that uses it does the trick: $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...] QEMU 0.12.50 monitor - type 'help' for more information (qemu) info snapshots No available block device supports snapshots (qemu) drive_add auto if=none,file=tmp.qcow2 OK (qemu) device_add usb-storage,id=foo,drive=none1 (qemu) info snapshots Snapshot devices: none1 Snapshot list (from none1): ID TAG VM SIZE DATE VM CLOCK (qemu) device_del foo (qemu) info snapshots Snapshot devices: Segmentation fault (core dumped) Move management of that pointer to block.c, and zap it when the device it points to goes away. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 25 +++++++++++++++++++++++++ block.h | 1 + savevm.c | 31 ++++--------------------------- 3 files changed, 30 insertions(+), 27 deletions(-)