| Message ID | 20210504082553.20377-10-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: refactor write threshold | expand |
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. Max
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? For me, if statement can be removed with no effect, it's an extra statement.
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. > >
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" -#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"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/write-threshold.c | 3 --- 1 file changed, 3 deletions(-)