Re: [Qemu-devel] [PATCH 1/1] block: fix libvirt snapshot with existing bitmaps

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]