Re: [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version of bdrv_release_dirty_bitmap

[John Snow]

On 11/16/2017 03:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2017 01:52, John Snow wrote:
>>
>> On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> It is needed to realize bdrv_dirty_bitmap_release_successor in
>>> the following patch.
>>>
>> OK, but...
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/dirty-bitmap.c | 25 ++++++++++++++++++++-----
>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 81adbeb6d4..981f99d362 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -326,13 +326,13 @@ static bool
>>> bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
>>>       return !!bdrv_dirty_bitmap_name(bitmap);
>>>   }
>>>   -/* Called with BQL taken.  */
>>> -static void bdrv_do_release_matching_dirty_bitmap(
>>> +/* Called within bdrv_dirty_bitmap_lock..unlock */
>> ...Add this so it will compile:
>>
>> __attribute__((__unused__))
> 
> ok
> 
>>> +static void bdrv_do_release_matching_dirty_bitmap_locked(
>>>       BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>>       bool (*cond)(BdrvDirtyBitmap *bitmap))
>>>   {
>>>       BdrvDirtyBitmap *bm, *next;
>>> -    bdrv_dirty_bitmaps_lock(bs);
>>> +
>>>       QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>>>           if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
>>>               assert(!bm->active_iterators);
>>> @@ -344,18 +344,33 @@ static void bdrv_do_release_matching_dirty_bitmap(
>>>               g_free(bm);
>>>                 if (bitmap) {
>>> -                goto out;
>>> +                return;
>>>               }
>>>           }
>>>       }
>>> +
>>>       if (bitmap) {
>>>           abort();
>>>       }
>> Do we have any style guide rules on using abort() instead of assert()?
>> The rest of this function uses assert, and it'd be less lines to simply
>> write:
>>
>> assert(!bitmap);
>>
>> which I think might also carry better semantic information for coverity
>> beyond an actual runtime conditional branch.
>>
>> (I think. Please correct me if I am wrong, I'm a little hazy on this.)
> 
> agree, but it is a preexisting code, so I'll fix it with an additional
> patch.
> 

You're right. I didn't notice it was pre-existing where I was looking at
it. I'll send my own little fixup. My mistake.

>>
>>> +}
>>>   -out:
>>> +/* Called with BQL taken.  */
>>> +static void bdrv_do_release_matching_dirty_bitmap(
>>> +    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>> +    bool (*cond)(BdrvDirtyBitmap *bitmap))
>>> +{
>>> +    bdrv_dirty_bitmaps_lock(bs);
>>> +    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
>>>       bdrv_dirty_bitmaps_unlock(bs);
>>>   }
>>>   +/* Called within bdrv_dirty_bitmap_lock..unlock */
>>> +static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
>>> +                                             BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
>>> +}
>>> +
>>>   /* Called with BQL taken.  */
>>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap)
>>>   {
>>>
>> If you agree with those two changes, you may add:
> 
> ok
> 
>>
>> Reviewed-by: John Snow <address@hidden>
> 
> 

Just make sure it compiles standalone by using the unused attribute and
you can add the RB.

--js