Re: [PATCH v3 0/3] block/dirty-bitmaps: fix and improve bitmap merge

[Vladimir Sementsov-Ogievskiy]

01.04.2022 23:17, Eric Blake wrote:
> On Fri, Apr 01, 2022 at 01:08:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > v3: rebase on master, one patch is already merged.
> >
> > Vladimir Sementsov-Ogievskiy (3):
> >    block: block_dirty_bitmap_merge(): fix error path
> >    block: improve block_dirty_bitmap_merge(): don't allocate extra bitmap
> >    block: simplify handling of try to merge different sized bitmaps
>
> Is any of this series worth getting into 7.0, or are we safe letting
> it slide to 7.1?

Let's check.

Only first patch is a fix.

anon bitmap is created on same bs where dst is found, so they should be 
compatible in size.

But bdrv_merge_dirty_bitmap also do some checks on dst status, which may 
actually fail..

So, in bad case we set errp, but return non-NULL dst bitmap.

Look at callers of block_dirty_bitmap_merge:

1. qmp_block_dirty_bitmap_merge() is OK, it ignores return value.

2. qmp_transaction use local_err to detect error, so we'll go through error path, 
that's good. state->bitmap is set, but it's not really matter. What makes sense is 
state->backup set or not?

state->backup is initialized with zero, as qmp_transaction() use g_malloc0 to 
allocate state buffer.

And bdrv_merge_dirty_bitmap() will do all checks prior to call 
bdrv_dirty_bitmap_merge_internal(), which actually can set @backup. So, 
state->backup is not set in our bad case.

So that all should be OK to postpone for 7.1.

--
Best regards,
Vladimir