Hello,
I have been through many kernel header files and have found that kernel header
files at many places don't include other header files which they have
dependency upon.
For example:
<linux/amba/bus.h> uses struct device and struct resource and it doesn't
include <linux/device.h> and <linux/resource.h> header files.
Now, whenever i try to include bus.h, i have to include device.h and resource.h.
Is this correct approach?
Again, if i include device.h and resource.h, they must be included before bus.h.
Now this will disturb the alphabetical ordering of including header files
sometimes. (not in this example)
Any idea behind this philosophy.
regards,
viresh kumar
ST Microelectronics
India.
From: viresh kumar <[email protected]>
Date: Tue, Feb 23, 2010 at 12:13:35PM +0530
Hi,
> I have been through many kernel header files and have found that kernel header
> files at many places don't include other header files which they have
> dependency upon.
>
> For example:
> <linux/amba/bus.h> uses struct device and struct resource and it doesn't
> include <linux/device.h> and <linux/resource.h> header files.
>
> Now, whenever i try to include bus.h, i have to include device.h and resource.h.
>
> Is this correct approach?
>
> Again, if i include device.h and resource.h, they must be included before bus.h.
and this is the thing: all those other files which include
<linux/amba/bus.h> either include <linux/device.h> and
<linux/resource.h> directly or the last are being included indirectly
through other headers.
Baseline, struct device and struct resource's definitions have to be
available before <linux/amba/bus.h> is included. That's why you have to
include the bus.h header last.
> Now this will disturb the alphabetical ordering of including header files
> sometimes. (not in this example)
I don't think there's such thing as alphabetical header ordering and if
it were it would be rather dumb thing to do.
Hope this helps.
--
Regards/Gruss,
Boris.
Hi,
>> Is this correct approach?
>>
>> Again, if i include device.h and resource.h, they must be included before bus.h.
>
> and this is the thing: all those other files which include
> <linux/amba/bus.h> either include <linux/device.h> and
> <linux/resource.h> directly or the last are being included indirectly
> through other headers.
>
> Baseline, struct device and struct resource's definitions have to be
> available before <linux/amba/bus.h> is included. That's why you have to
> include the bus.h header last.
>
We need to include device.h and resource.h at every place where we use bus.h.
Shouldn't it be responsibility of bus.h only? So that people don't
have to bother about bus.h
internal dependencies.
I think, ideally including any header file shouldn't give compilation
errors for types used in
included header file.
viresh kumar
On Tue, Feb 23, 2010 at 12:29 PM, Borislav Petkov
<[email protected]> wrote:
> From: viresh kumar <[email protected]>
> Date: Tue, Feb 23, 2010 at 12:13:35PM +0530
>
> Hi,
>
>> I have been through many kernel header files and have found that kernel header
>> files at many places don't include other header files which they have
>> dependency upon.
>>
>> For example:
>> <linux/amba/bus.h> uses struct device and struct resource and it doesn't
>> include <linux/device.h> and <linux/resource.h> header files.
>>
>> Now, whenever i try to include bus.h, i have to include device.h and resource.h.
>>
>> Is this correct approach?
>>
>> Again, if i include device.h and resource.h, they must be included before bus.h.
>
> and this is the thing: all those other files which include
> <linux/amba/bus.h> either include <linux/device.h> and
> <linux/resource.h> directly or the last are being included indirectly
> through other headers.
>
> Baseline, struct device and struct resource's definitions have to be
> available before <linux/amba/bus.h> is included. That's why you have to
> include the bus.h header last.
>
>> Now this will disturb the alphabetical ordering of including header files
>> sometimes. (not in this example)
>
> I don't think there's such thing as alphabetical header ordering and if
> it were it would be rather dumb thing to do.
>
> Hope this helps.
>
> --
> Regards/Gruss,
> ? ?Boris.
>
From: viresh kumar <[email protected]>
Date: Tue, Feb 23, 2010 at 01:01:25PM +0530
> >> Again, if i include device.h and resource.h, they must be included before bus.h.
> >
> > and this is the thing: all those other files which include
> > <linux/amba/bus.h> either include <linux/device.h> and
> > <linux/resource.h> directly or the last are being included indirectly
> > through other headers.
> >
> > Baseline, struct device and struct resource's definitions have to be
> > available before <linux/amba/bus.h> is included. That's why you have to
> > include the bus.h header last.
> >
>
> We need to include device.h and resource.h at every place where we use bus.h.
>
> Shouldn't it be responsibility of bus.h only? So that people don't
> have to bother about bus.h
> internal dependencies.
A quick grep in <include/linux/> reveals that most, if not all, of the
headers that use struct resource, for example, include ioport.h which
contains the definition. So yes, it should be like this, besides we
guard against multiple inclusion with the #ifndef ..., #define... #endif
thing anyway.
> I think, ideally including any header file shouldn't give compilation
> errors for types used in
> included header file.
Agreed.
I'd send a patch fixing the bus.h header, in case no one has a valid
technical reason against it.
--
Regards/Gruss,
Boris.
>> I think, ideally including any header file shouldn't give compilation
>> errors for types used in
>> included header file.
>
> Agreed.
>
> I'd send a patch fixing the bus.h header, in case no one has a valid
> technical reason against it.
>
That will be great!!!
Actually this issue is not present only in bus.h, but some other
kernel header files.
Like: arch/arm/include/asm/clkdev.h don't include list.h file but
using struct list_head
May be we need to check this in other header files also.
regards,
viresh kumar
From: viresh kumar <[email protected]>
Date: Tue, Feb 23, 2010 at 05:07:47PM +0530
> >> I think, ideally including any header file shouldn't give compilation
> >> errors for types used in
> >> included header file.
> >
> > Agreed.
> >
> > I'd send a patch fixing the bus.h header, in case no one has a valid
> > technical reason against it.
> >
>
> That will be great!!!
Just to make sure: with "I'd" I meant "I would", i.e. actually _you_
could send a patch fixing that by explaining the problem in the commit
message :).
> Actually this issue is not present only in bus.h, but some other
> kernel header files.
> Like: arch/arm/include/asm/clkdev.h don't include list.h file but
> using struct list_head
>
> May be we need to check this in other header files also.
Well, you should talk to the arm maintainer about that task and whether
it is desirable.
--
Regards/Gruss,
Boris.
Borislav Petkov wrote:
> From: viresh kumar <[email protected]>
>> Actually this issue is not present only in bus.h, but some other
>> kernel header files.
>> Like: arch/arm/include/asm/clkdev.h don't include list.h file but
>> using struct list_head
>>
>> May be we need to check this in other header files also.
>
> Well, you should talk to the arm maintainer about that task and whether
> it is desirable.
I agree but feel compelled to add: While each header file should indeed
include everything that is necessary to allow for arbitrary orders of
inclusion of this header,? this is sometimes not possible for core
kernel headers or architecture headers.
An example over which I stumbled a few days ago: linux/wait.h cannot
easily include linux/sched.h although it uses definitions from it.
There is a direct circular dependency which can be easily resolved, but
there are also dependencies at deeper levels of indirection which cannot
be easily resolved.
----------
?) IOW each header should include everything which it uses. OTOH a
user of that header should not rely on having its own dependencies
included indirectly.
--
Stefan Richter
-=====-==-=- --=- =-===
http://arcgraph.de/sr/
On Feb 23, 2010, at 6:37 AM, viresh kumar wrote:
>
> Actually this issue is not present only in bus.h, but some other
> kernel header files.
> Like: arch/arm/include/asm/clkdev.h don't include list.h file but
> using struct list_head
>
> May be we need to check this in other header files also.
Before someone goes crazy and starts sending hundreds of patches to the trivial patch folks, please make sure that you only do this for places where header file foo uses "struct bar" in bar.h --- and NOT if it is using "struct bar *". Blind structure pointers don't cause compile failures, and is a perfectly good thing from the standpoint of data hiding.
Also, it's highly desirable that as much as possible multiple inclusion is fixed up at the same time you add extra #includes into header files. Protecting against multiple inclusion is critical, yes, but even with the protection against multiple inclusion, the header file has to get parsed a second time, and that slows down kernel compiles.
Regards,
-- Ted
> Before someone goes crazy and starts sending hundreds of patches to the trivial patch folks,
> please make sure that you only do this for places where header file foo uses "struct bar" in
>bar.h --- and NOT if it is using "struct bar *". Blind structure pointers don't cause compile
>failures, and is a perfectly good thing from the standpoint of data hiding.
>
Ted,
Actually bus.h is using direct instances of these structures and thus
it is giving compilation warnings.
viresh.
> Just to make sure: with "I'd" I meant "I would", i.e. actually _you_
> could send a patch fixing that by explaining the problem in the commit
> message :).
>
Yes boris, i will do that if everybody agrees.
>> Actually this issue is not present only in bus.h, but some other
>> kernel header files.
>> Like: arch/arm/include/asm/clkdev.h don't include list.h file but
>> using struct list_head
>>
> Well, you should talk to the arm maintainer about that task and whether
> it is desirable.
i will float a patch for the same in linux-arm mailing list.
viresh.
On Feb 23, 2010, at 11:28 PM, viresh kumar wrote:
>> Before someone goes crazy and starts sending hundreds of patches to the trivial patch folks,
>> please make sure that you only do this for places where header file foo uses "struct bar" in
>> bar.h --- and NOT if it is using "struct bar *". Blind structure pointers don't cause compile
>> failures, and is a perfectly good thing from the standpoint of data hiding.
>>
>
> Ted,
> Actually bus.h is using direct instances of these structures and thus
> it is giving compilation warnings.
I'm aware of that --- in this case. However, this would not be the first time some over-eager kernel programmer newbie finds a message in LKML, reads it out of context, and then starts sending large number of useless, and even potentially harmful patches which all of the kernel maintainers then have to check and NACK.
So while I was pretty sure you knew what you meant, the fact that other people were saying, "Yeah! Let's sweep through the header files finding all of these potential problems and clean them up," sent a chill down my spine.
-- Ted
Theodore Tso wrote:
> Also, it's highly desirable that as much as possible multiple
> inclusion is fixed up at the same time you add extra #includes into
> header files. Protecting against multiple inclusion is critical,
> yes, but even with the protection against multiple inclusion, the
> header file has to get parsed a second time, and that slows down
> kernel compiles.
Multiple inclusion of a protected header does not hurt at all; gcc
detects that a header file uses #ifdef/#endif header guards and
automatically ignores any #include for that file if the symbol is
already defined.
Regards,
Clemens