Re: [PATCH v2 9/9] block/write-threshold: drop extra includes

[Max Reitz <mreitz@redhat.com>]

On 05.05.21 22:34, Vladimir Sementsov-Ogievskiy wrote:
> 05.05.2021 19:23, Max Reitz wrote:
>> On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/write-threshold.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/block/write-threshold.c b/block/write-threshold.c
>>> index fbf4e6f5c4..db271c5537 100644
>>> --- a/block/write-threshold.c
>>> +++ b/block/write-threshold.c
>>> @@ -12,10 +12,7 @@
>>>    */
>>>   #include "qemu/osdep.h"
>>> -#include "block/block_int.h"
>>
>> We need this (for bs->write_threshold_offset), but it’s included 
>> through block/block_int.h.  I’m not sure whether we should drop it 
>> from the includes.
>>
>> Perhaps we should instead drop block_int.h from write-threshold.h? 
>> Shouldn’t including qemu/osdep.h (which includes qemu/typedefs.h, 
>> which forward-declares BDS) be sufficient there?
>>
>>> -#include "qemu/coroutine.h"
>>>   #include "block/write-threshold.h"
>>> -#include "qemu/notify.h"
>>>   #include "qapi/error.h"
>>>   #include "qapi/qapi-commands-block-core.h"
>>>   #include "qapi/qapi-events-block-core.h"
>>
>> Btw, where does qemu/atomic.h come in?  Looks like it comes through 
>> block_int.h.  I think we should include it explicitly.
>>
> 
> I'm not sure. If something is included through another include, why to 
> include it explicitly?

Because the other include may change.  I’d include something if you need 
the feature, and we need atomics here.

And block_int.h doesn’t even provide atomic.h, it comes through various 
of its includes (which are probably not included to provide atomics). 
So this is already indirect and basically just incidental; block_int.h 
doesn’t guarantee atomic.h.

Another thing: I see that other fields in BDS that are to be accessed 
with atomic ops have a comment saying so.  I think 
write_threshold_offset should have, too.

Max

> For me, if statement can be removed with no effect, it's an extra 
> statement.
> 
>