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

[Max Reitz]

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.