| Message ID | 1465821535-6664-1-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13.06.2016 14:38, Denis V. Lunev wrote: > From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Fix the following bug: > > # virsh start test > Domain test started > > # virsh qemu-monitor-command test \ > '{"execute":"block-dirty-bitmap-add",\ > "arguments":{"node":"drive0","name":"ab"}}' > {"return":{},"id":"libvirt-36"}'}' > > # virsh snapshot-create test > error: Unable to read from monitor: Connection reset by peer > > Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires, > because qcow2_save_vmstate just writes to bs (not to bs->file->bs) after > the end of the drive. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > --- > block/dirty-bitmap.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 4902ca5..7b636ee 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -364,6 +364,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > int nr_sectors) > { > BdrvDirtyBitmap *bitmap; > + > + if (cur_sector >= bdrv_nb_sectors(bs)) { > + return; > + } I'd use bitmap->size instead of bdrv_nb_sectors(bs). In addition, I'd put an assert(cur_sector + nr_sectors <= bitmap->size) after this conditional block; I understand that we will probably never write to both the disk and the VM state in a single operation, therefore asserting this is sufficient. (If we want to do it right, we'd need to truncate nr_sectors in case cur_sector < bdrv_nb_sectors(bs) && cur_sector + nr_sectors > bitmap->size) > + > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > if (!bdrv_dirty_bitmap_enabled(bitmap)) { > continue; > Apart from that: I see that this is the only place where it's really relevant. However, I feel somewhat bad putting it just here and not in the other functions (bdrv_{re,}set_dirty_bitmap() and bdrv_get_dirty()). I just want to bring this to your attention. Technically, putting the check into this function is completely sufficient, so if you think it's too much work to put it into the other ones, I'm fine with that (although I think we should at least put an assertion into those other functions). Max
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 4902ca5..7b636ee 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -364,6 +364,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors) { BdrvDirtyBitmap *bitmap; + + if (cur_sector >= bdrv_nb_sectors(bs)) { + return; + } + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { if (!bdrv_dirty_bitmap_enabled(bitmap)) { continue;