qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 1/1] block: fix libvirt snapshot with existing bitmaps
Date: Tue, 14 Jun 2016 16:12:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 13.06.2016 14:38, Denis V. Lunev wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> ---
>  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: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]