[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version of bdrv_release_dirty_bitmap |
Date: |
Thu, 16 Nov 2017 13:18:48 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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