LinuxLists.cc - [GIT PULL -tip] fix 41 'make headers

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Sun, 2009-01-18 at 12:02 +0100, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > index 882dc72..a20c97c 100644
> > --- a/include/linux/acct.h
> > +++ b/include/linux/acct.h
> > @@ -59,9 +59,13 @@ struct acct
> > comp_t ac_majflt; /* Major Pagefaults */
> > comp_t ac_swaps; /* Number of Swaps */
> > /* m68k had no padding here. */
> > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > +#ifdef __KERNEL__
> > +#ifndef CONFIG_M68K
> > __u16 ac_ahz; /* AHZ */
> > -#endif
> > +#endif /* CONFIG_M68K */
> > +#else /* __KERNEL__ */
> > + __u16 ac_ahz; /* AHZ */
> > +#endif /* __KERNEL__ */
>
> that looks rather ugly.
>
> Why not just flip it around to:
>
> #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
>
> ? Does headers_check misinterpret that?
>

This will not many any difference:
then usr/include/linux/acct.h will look like:
#if !defined(__KERNEL__) || !defined(CONFIG_M68K)
__u16 ac_ahz; /* AHZ */
#endif

And we will get same warning:
usr/include/linux/acct.h:62: leaks CONFIG_M68K to userspace where it is not valid

> > * To make everything easier to port and manage cross platform
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 343df9e..1202063 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -528,6 +528,7 @@ struct sysinfo {
> > /* Trap pasters of __FUNCTION__ at compile-time */
> > #define __FUNCTION__ (__func__)
> >
> > +#ifdef __KERNEL__
> > /* This helps us to avoid #ifdef CONFIG_NUMA */
> > #ifdef CONFIG_NUMA
> > #define NUMA_BUILD 1
> > @@ -540,4 +541,8 @@ struct sysinfo {
> > # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
> > #endif
> >
> > +#else /* __KERNEL__ */
> > +#define NUMA_BUILD 0
> > +#endif /* __KERNEL__ */
>
> Does NUMA_BUILD make any sense to user-space at all? Shouldnt we leave it
> undefined?
>

OK, I can do this.

> > --- a/include/linux/pktcdvd.h
> > +++ b/include/linux/pktcdvd.h
> > @@ -33,11 +33,15 @@
> > * able to sucessfully recover with this option (drive will return good
> > * status as soon as the cdb is validated).
> > */
> > +#ifdef __KERNEL__
> > #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> > #define USE_WCACHING 1
> > #else
> > #define USE_WCACHING 0
> > #endif
> > +#else /* __KERNEL__ */
> > +#define USE_WCACHING 0
> > +#endif /* __KERNEL__ */
>
> does USE_WCACHING make any sense to user-space? Shouldnt we leave it
> undefined?
>

Sure.

> > diff --git a/include/linux/raw.h b/include/linux/raw.h
> > index 62d543e..3898e30 100644
> > --- a/include/linux/raw.h
> > +++ b/include/linux/raw.h
> > @@ -13,6 +13,10 @@ struct raw_config_request
> > __u64 block_minor;
> > };
> >
> > +#ifdef __KERNEL__
> > #define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
> > +#else /* __KERNEL__ */
> > +#define MAX_RAW_MINORS 0
> > +#endif /* __KERNEL__ */
>
> ditto.
>

OK.

> > #endif /* __LINUX_RAW_H */
> > diff --git a/include/linux/socket.h b/include/linux/socket.h
> > index f5771a2..d7daa52 100644
> > --- a/include/linux/socket.h
> > +++ b/include/linux/socket.h
> > @@ -256,11 +256,15 @@ struct ucred {
> > #define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exit for file
> > descriptor received through
> > SCM_RIGHTS */
> > +#ifdef __KERNEL__
> > #if defined(CONFIG_COMPAT)
> > #define MSG_CMSG_COMPAT 0x80000000 /* This message needs 32 bit fixups */
> > #else
> > #define MSG_CMSG_COMPAT 0 /* We never have 32 bit fixups */
> > #endif
> > +#else /* __KERNEL__ */
> > +#define MSG_CMSG_COMPAT 0 /* We never have 32 bit fixups */
> > +#endif /* __KERNEL__ */
>
> I suspect this flag should always be defined for user-space - the zero
> value only makes sense in the kernel.
>

OK.

> > --- a/include/linux/types.h
> > +++ b/include/linux/types.h
> > @@ -138,6 +138,7 @@ typedef __s64 int64_t;
> > *
> > * blkcnt_t is the type of the inode's block count.
> > */
> > +#ifdef __KERNEL__
> > #ifdef CONFIG_LBD
> > typedef u64 sector_t;
> > typedef u64 blkcnt_t;
> > @@ -145,6 +146,10 @@ typedef u64 blkcnt_t;
> > typedef unsigned long sector_t;
> > typedef unsigned long blkcnt_t;
> > #endif
> > +#else /* __KERNEL__ */
> > +typedef unsigned long sector_t;
> > +typedef unsigned long blkcnt_t;
> > +#endif /* __KERNEL__ */
>
> heh. types.h itself is not headers_check clean.
>
> But this isnt particularly clean: we have now 3 blocks of typedefs while
> there are just 2 variants. It would be cleaner to do something like:
>
> #if !defined(__KERNEL__) || defined(CONFIG_LBD)
>
> i.e. always provide the wider type to user-space.

Sorry, this will not be helpful we will still get the warning.

Now we need to decide, should we manage with 3 blocks or should we stay
with warnings.

But usr/include/types.h looks pretty clean:
/**
* The type used for indexing onto a disc or disc partition.
*
* Linux always considers sectors to be 512 bytes long independently
* of the devices real block size.
*
* blkcnt_t is the type of the inode's block count.
*/
typedef unsigned long sector_t;
typedef unsigned long blkcnt_t;

--
JSR

2009-01-18 12:26:43

by Ingo Molnar

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

* Jaswinder Singh Rajput <[email protected]> wrote:

> On Sun, 2009-01-18 at 12:02 +0100, Ingo Molnar wrote:
> > * Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > > index 882dc72..a20c97c 100644
> > > --- a/include/linux/acct.h
> > > +++ b/include/linux/acct.h
> > > @@ -59,9 +59,13 @@ struct acct
> > > comp_t ac_majflt; /* Major Pagefaults */
> > > comp_t ac_swaps; /* Number of Swaps */
> > > /* m68k had no padding here. */
> > > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > > +#ifdef __KERNEL__
> > > +#ifndef CONFIG_M68K
> > > __u16 ac_ahz; /* AHZ */
> > > -#endif
> > > +#endif /* CONFIG_M68K */
> > > +#else /* __KERNEL__ */
> > > + __u16 ac_ahz; /* AHZ */
> > > +#endif /* __KERNEL__ */
> >
> > that looks rather ugly.
> >
> > Why not just flip it around to:
> >
> > #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
> >
> > ? Does headers_check misinterpret that?
> >
>
> This will not many any difference:
> then usr/include/linux/acct.h will look like:
> #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
> __u16 ac_ahz; /* AHZ */
> #endif
>
> And we will get same warning:
> usr/include/linux/acct.h:62: leaks CONFIG_M68K to userspace where it is not valid

which would be a false positive. Sam, what would be your preference to
annotate the code in such cases?

> > i.e. always provide the wider type to user-space.
>
> Sorry, this will not be helpful we will still get the warning.

the problem here is that the type you export to user-space is the _wrong_
(narrow) type. Including types.h is one thing - exporting something to
user-space is another thing. It might not matter in practice.

Furthermore, the 3 blocks are there as well - while there should only be
two.

Ingo

2009-01-18 12:56:13

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Sun, Jan 18, 2009 at 12:02:21PM +0100, Ingo Molnar wrote:
>
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > index 882dc72..a20c97c 100644
> > --- a/include/linux/acct.h
> > +++ b/include/linux/acct.h
> > @@ -59,9 +59,13 @@ struct acct
> > comp_t ac_majflt; /* Major Pagefaults */
> > comp_t ac_swaps; /* Number of Swaps */
> > /* m68k had no padding here. */
> > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > +#ifdef __KERNEL__
> > +#ifndef CONFIG_M68K
> > __u16 ac_ahz; /* AHZ */
> > -#endif
> > +#endif /* CONFIG_M68K */
> > +#else /* __KERNEL__ */
> > + __u16 ac_ahz; /* AHZ */
> > +#endif /* __KERNEL__ */
>
> that looks rather ugly.
>
> Why not just flip it around to:
>
> #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
>
> ? Does headers_check misinterpret that?

The original expression is misinterpreted by headers_check
because we want the ac_ahz to stay if either of __KERNEL__
or CONFIG_M68K is not defined.
And unifdef does not optimize away the !defined(CONFIG_M68K)
part - it has no knowledge that this is kernel internal.

So I am happy with Jaswinder's patch.

That said I really no not understand why there is this
subtle issue with struct acct in the first place..

Sam

2009-01-18 12:59:25

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Sun, Jan 18, 2009 at 01:57:22PM +0100, Sam Ravnborg wrote:
> On Sun, Jan 18, 2009 at 12:02:21PM +0100, Ingo Molnar wrote:
> >
> > * Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > > index 882dc72..a20c97c 100644
> > > --- a/include/linux/acct.h
> > > +++ b/include/linux/acct.h
> > > @@ -59,9 +59,13 @@ struct acct
> > > comp_t ac_majflt; /* Major Pagefaults */
> > > comp_t ac_swaps; /* Number of Swaps */
> > > /* m68k had no padding here. */
> > > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > > +#ifdef __KERNEL__
> > > +#ifndef CONFIG_M68K
> > > __u16 ac_ahz; /* AHZ */
> > > -#endif
> > > +#endif /* CONFIG_M68K */
> > > +#else /* __KERNEL__ */
> > > + __u16 ac_ahz; /* AHZ */
> > > +#endif /* __KERNEL__ */
> >
> > that looks rather ugly.
> >
> > Why not just flip it around to:
> >
> > #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
> >
> > ? Does headers_check misinterpret that?
>
> The original expression is misinterpreted by headers_check
> because we want the ac_ahz to stay if either of __KERNEL__
> or CONFIG_M68K is not defined.
> And unifdef does not optimize away the !defined(CONFIG_M68K)
> part - it has no knowledge that this is kernel internal.
>
> So I am happy with Jaswinder's patch.
Almost happy.
I digged out my original patch and it looks like this:
-#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
+#ifndef __KERNEL__
__u16 ac_ahz; /* AHZ */
+#else
+ #ifndef CONFIG_M68K
+ __u16 ac_ahz; /* AHZ */
+ #endif
#endif

The indention can be discussed..
But the logic is simpler.

Sam

2009-01-18 17:29:39

by Ingo Molnar

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

* Sam Ravnborg <[email protected]> wrote:

> On Sun, Jan 18, 2009 at 01:57:22PM +0100, Sam Ravnborg wrote:
> > On Sun, Jan 18, 2009 at 12:02:21PM +0100, Ingo Molnar wrote:
> > >
> > > * Jaswinder Singh Rajput <[email protected]> wrote:
> > >
> > > > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > > > index 882dc72..a20c97c 100644
> > > > --- a/include/linux/acct.h
> > > > +++ b/include/linux/acct.h
> > > > @@ -59,9 +59,13 @@ struct acct
> > > > comp_t ac_majflt; /* Major Pagefaults */
> > > > comp_t ac_swaps; /* Number of Swaps */
> > > > /* m68k had no padding here. */
> > > > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > > > +#ifdef __KERNEL__
> > > > +#ifndef CONFIG_M68K
> > > > __u16 ac_ahz; /* AHZ */
> > > > -#endif
> > > > +#endif /* CONFIG_M68K */
> > > > +#else /* __KERNEL__ */
> > > > + __u16 ac_ahz; /* AHZ */
> > > > +#endif /* __KERNEL__ */
> > >
> > > that looks rather ugly.
> > >
> > > Why not just flip it around to:
> > >
> > > #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
> > >
> > > ? Does headers_check misinterpret that?
> >
> > The original expression is misinterpreted by headers_check
> > because we want the ac_ahz to stay if either of __KERNEL__
> > or CONFIG_M68K is not defined.
> > And unifdef does not optimize away the !defined(CONFIG_M68K)
> > part - it has no knowledge that this is kernel internal.
> >
> > So I am happy with Jaswinder's patch.
> Almost happy.
> I digged out my original patch and it looks like this:
> -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> +#ifndef __KERNEL__
> __u16 ac_ahz; /* AHZ */
> +#else
> + #ifndef CONFIG_M68K
> + __u16 ac_ahz; /* AHZ */
> + #endif
> #endif
>
> The indention can be discussed..
> But the logic is simpler.

yes - that's the 3-block versus 2-block issue i mentioned to Jaswinder.
(the first version duplicated the same thing into 3 places - while the
logic only splits it in two)

2009-01-18 22:40:27

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 5571dbe..81fa255 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1480,7 +1480,7 @@ struct v4l2_chip_ident_old {
>
> #if 1
> /* Experimental, meant for debugging, testing and internal use.
> - Only implemented if CONFIG_VIDEO_ADV_DEBUG is defined.
> + Only implemented if VIDEO_ADV_DEBUG is defined.
> You must be root to use these ioctls. Never use these in applications! */

Is there some reason we cannot fix the tool that does the checking for
this case?

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (789.00 B)
(No filename) (197.00 B)
Download all attachments

2009-01-18 22:51:00

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
> diff --git a/include/linux/raw.h b/include/linux/raw.h
> index 62d543e..3898e30 100644
> --- a/include/linux/raw.h
> +++ b/include/linux/raw.h
> @@ -13,6 +13,10 @@ struct raw_config_request
> __u64 block_minor;
> };
>
> +#ifdef __KERNEL__
> #define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
> +#else /* __KERNEL__ */
> +#define MAX_RAW_MINORS 0
> +#endif /* __KERNEL__ */
>
> #endif /* __LINUX_RAW_H */

Since MAX_RAW_MINORS has only one user (drivers/char/raw.c), why not
just remove the define from here and add it it there?

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (740.00 B)
(No filename) (197.00 B)
Download all attachments

2009-01-18 23:53:41

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
> diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
> index 04b4d73..277de8c 100644
> --- a/include/linux/pktcdvd.h
> +++ b/include/linux/pktcdvd.h
> @@ -33,11 +33,15 @@
> * able to sucessfully recover with this option (drive will return good
> * status as soon as the cdb is validated).
> */
> +#ifdef __KERNEL__
> #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> #define USE_WCACHING 1
> #else
> #define USE_WCACHING 0
> #endif
> +#else /* __KERNEL__ */
> +#define USE_WCACHING 0
> +#endif /* __KERNEL__ */

This also only has one user (drivers/block/pktcdvd.c) so maybe it should
be moved there.

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (829.00 B)
(No filename) (197.00 B)
Download all attachments

2009-01-19 00:00:25

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
>
> /home/jaswinder/jaswinder-git/linux-2.6-tip/usr/include/linux/pkt_cls.h:306: leaks CONFIG_NET to userspace where it is not valid
> /home/jaswinder/jaswinder-git/linux-2.6-tip/usr/include/linux/pkt_cls.h:307: leaks CONFIG_NET to userspace where it is not valid

> diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h
> index 3c842ed..7a45e09 100644
> --- a/include/linux/pkt_cls.h
> +++ b/include/linux/pkt_cls.h
> @@ -304,8 +304,8 @@ enum
> TCA_FW_UNSPEC,
> TCA_FW_CLASSID,
> TCA_FW_POLICE,
> - TCA_FW_INDEV, /* used by CONFIG_NET_CLS_IND */
> - TCA_FW_ACT, /* used by CONFIG_NET_CLS_ACT */
> + TCA_FW_INDEV, /* used by NET_CLS_IND */
> + TCA_FW_ACT, /* used by NET_CLS_ACT */
> TCA_FW_MASK,

Again, surely we can improve the tool to not warn about these. If nothing
else, the actual warning is not correct.

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (1.02 kB)
(No filename) (197.00 B)
Download all attachments

2009-01-19 01:18:53

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Mon, Jan 19, 2009 at 09:39:59AM +1100, Stephen Rothwell wrote:
> Hi Jaswinder,
>
> On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 5571dbe..81fa255 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1480,7 +1480,7 @@ struct v4l2_chip_ident_old {
> >
> > #if 1
> > /* Experimental, meant for debugging, testing and internal use.
> > - Only implemented if CONFIG_VIDEO_ADV_DEBUG is defined.
> > + Only implemented if VIDEO_ADV_DEBUG is defined.
> > You must be root to use these ioctls. Never use these in applications! */
>
> Is there some reason we cannot fix the tool that does the checking for
> this case?

It could be done - but for now I have kept it very simple.
One day I should preprocess the headers or something to cover more
issues.

Sam

2009-01-19 02:31:35

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Mon, 2009-01-19 at 09:50 +1100, Stephen Rothwell wrote:
> Hi Jaswinder,
>
> On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > diff --git a/include/linux/raw.h b/include/linux/raw.h
> > index 62d543e..3898e30 100644
> > --- a/include/linux/raw.h
> > +++ b/include/linux/raw.h
> > @@ -13,6 +13,10 @@ struct raw_config_request
> > __u64 block_minor;
> > };
> >
> > +#ifdef __KERNEL__
> > #define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
> > +#else /* __KERNEL__ */
> > +#define MAX_RAW_MINORS 0
> > +#endif /* __KERNEL__ */
> >
> > #endif /* __LINUX_RAW_H */
>
> Since MAX_RAW_MINORS has only one user (drivers/char/raw.c), why not
> just remove the define from here and add it it there?
>

Already fixed in v3 as pointed by Ingo.

2009-01-19 02:32:07

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Mon, 2009-01-19 at 10:53 +1100, Stephen Rothwell wrote:
> Hi Jaswinder,
>
> On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
> > index 04b4d73..277de8c 100644
> > --- a/include/linux/pktcdvd.h
> > +++ b/include/linux/pktcdvd.h
> > @@ -33,11 +33,15 @@
> > * able to sucessfully recover with this option (drive will return good
> > * status as soon as the cdb is validated).
> > */
> > +#ifdef __KERNEL__
> > #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> > #define USE_WCACHING 1
> > #else
> > #define USE_WCACHING 0
> > #endif
> > +#else /* __KERNEL__ */
> > +#define USE_WCACHING 0
> > +#endif /* __KERNEL__ */
>
> This also only has one user (drivers/block/pktcdvd.c) so maybe it should
> be moved there.
>

Already fixed in v3 as pointed by Ingo.

Thanks,

--
JSR

2009-01-19 03:48:28

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Mon, 19 Jan 2009 08:01:14 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
> On Mon, 2009-01-19 at 10:53 +1100, Stephen Rothwell wrote:
> >
> > On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
> > >
> > > diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
> > > index 04b4d73..277de8c 100644
> > > --- a/include/linux/pktcdvd.h
> > > +++ b/include/linux/pktcdvd.h
> > > @@ -33,11 +33,15 @@
> > > * able to sucessfully recover with this option (drive will return good
> > > * status as soon as the cdb is validated).
> > > */
> > > +#ifdef __KERNEL__
> > > #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> > > #define USE_WCACHING 1
> > > #else
> > > #define USE_WCACHING 0
> > > #endif
> > > +#else /* __KERNEL__ */
> > > +#define USE_WCACHING 0
> > > +#endif /* __KERNEL__ */
> >
> > This also only has one user (drivers/block/pktcdvd.c) so maybe it should
> > be moved there.
> >
>
> Already fixed in v3 as pointed by Ingo.

I was actually suggesting that you move the whole

#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
#define USE_WCACHING 1
#else
#define USE_WCACHING 0
#endif

into drivers/block/pktcdvd.c as that is the only place USE_WCACHING is
used.

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (1.32 kB)
(No filename) (197.00 B)
Download all attachments

2009-01-19 03:54:03

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hi Jaswinder,

On Mon, 19 Jan 2009 08:00:40 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
>
> On Mon, 2009-01-19 at 09:50 +1100, Stephen Rothwell wrote:
> > Hi Jaswinder,
> >
> > On Sun, 18 Jan 2009 15:40:54 +0530 Jaswinder Singh Rajput <[email protected]> wrote:
> > >
> > > diff --git a/include/linux/raw.h b/include/linux/raw.h
> > > index 62d543e..3898e30 100644
> > > --- a/include/linux/raw.h
> > > +++ b/include/linux/raw.h
> > > @@ -13,6 +13,10 @@ struct raw_config_request
> > > __u64 block_minor;
> > > };
> > >
> > > +#ifdef __KERNEL__
> > > #define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
> > > +#else /* __KERNEL__ */
> > > +#define MAX_RAW_MINORS 0
> > > +#endif /* __KERNEL__ */
> > >
> > > #endif /* __LINUX_RAW_H */
> >
> > Since MAX_RAW_MINORS has only one user (drivers/char/raw.c), why not
> > just remove the define from here and add it it there?
> >
>
> Already fixed in v3 as pointed by Ingo.

Again I was suggesting that the whole #define line could be moved to the
only C files it is used in.

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (1.12 kB)
(No filename) (197.00 B)
Download all attachments

2009-01-19 05:16:54

by Jaswinder Singh

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hello Stephen,

On Mon, Jan 19, 2009 at 9:18 AM, Stephen Rothwell <[email protected]> wrote:
>> > >
>> > > diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
>> > > index 04b4d73..277de8c 100644
>> > > --- a/include/linux/pktcdvd.h
>> > > +++ b/include/linux/pktcdvd.h
>> > > @@ -33,11 +33,15 @@
>> > > * able to sucessfully recover with this option (drive will return good
>> > > * status as soon as the cdb is validated).
>> > > */
>> > > +#ifdef __KERNEL__
>> > > #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
>> > > #define USE_WCACHING 1
>> > > #else
>> > > #define USE_WCACHING 0
>> > > #endif
>> > > +#else /* __KERNEL__ */
>> > > +#define USE_WCACHING 0
>> > > +#endif /* __KERNEL__ */
>> >
>> > This also only has one user (drivers/block/pktcdvd.c) so maybe it should
>> > be moved there.
>> >
>>
>> Already fixed in v3 as pointed by Ingo.
>
> I was actually suggesting that you move the whole
>
> #if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> #define USE_WCACHING 1
> #else
> #define USE_WCACHING 0
> #endif
>
> into drivers/block/pktcdvd.c as that is the only place USE_WCACHING is
> used.
>

hmm, this is the out of scope of this patch and should be send in
different patch.

I think maintainers can handle this issue.

If they need my help, they can ping me ;-)

Thanks,

JSR

2009-01-21 00:59:19

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

On Sun, 2009-01-18 at 18:29 +0100, Ingo Molnar wrote:
> * Sam Ravnborg <[email protected]> wrote:
>
> > On Sun, Jan 18, 2009 at 01:57:22PM +0100, Sam Ravnborg wrote:
> > > On Sun, Jan 18, 2009 at 12:02:21PM +0100, Ingo Molnar wrote:
> > > >
> > > > * Jaswinder Singh Rajput <[email protected]> wrote:
> > > >
> > > > > diff --git a/include/linux/acct.h b/include/linux/acct.h
> > > > > index 882dc72..a20c97c 100644
> > > > > --- a/include/linux/acct.h
> > > > > +++ b/include/linux/acct.h
> > > > > @@ -59,9 +59,13 @@ struct acct
> > > > > comp_t ac_majflt; /* Major Pagefaults */
> > > > > comp_t ac_swaps; /* Number of Swaps */
> > > > > /* m68k had no padding here. */
> > > > > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > > > > +#ifdef __KERNEL__
> > > > > +#ifndef CONFIG_M68K
> > > > > __u16 ac_ahz; /* AHZ */
> > > > > -#endif
> > > > > +#endif /* CONFIG_M68K */
> > > > > +#else /* __KERNEL__ */
> > > > > + __u16 ac_ahz; /* AHZ */
> > > > > +#endif /* __KERNEL__ */
> > > >
> > > > that looks rather ugly.
> > > >
> > > > Why not just flip it around to:
> > > >
> > > > #if !defined(__KERNEL__) || !defined(CONFIG_M68K)
> > > >
> > > > ? Does headers_check misinterpret that?
> > >
> > > The original expression is misinterpreted by headers_check
> > > because we want the ac_ahz to stay if either of __KERNEL__
> > > or CONFIG_M68K is not defined.
> > > And unifdef does not optimize away the !defined(CONFIG_M68K)
> > > part - it has no knowledge that this is kernel internal.
> > >
> > > So I am happy with Jaswinder's patch.
> > Almost happy.
> > I digged out my original patch and it looks like this:
> > -#if !defined(CONFIG_M68K) || !defined(__KERNEL__)
> > +#ifndef __KERNEL__
> > __u16 ac_ahz; /* AHZ */
> > +#else
> > + #ifndef CONFIG_M68K
> > + __u16 ac_ahz; /* AHZ */
> > + #endif
> > #endif
> >
> > The indention can be discussed..
> > But the logic is simpler.
>
> yes - that's the 3-block versus 2-block issue i mentioned to Jaswinder.
> (the first version duplicated the same thing into 3 places - while the
> logic only splits it in two)

my 7 lines patch was:
#ifdef __KERNEL__
#ifndef CONFIG_M68K
__u16 ac_ahz; /* AHZ */
#endif /* CONFIG_M68K */
#else /* __KERNEL__ */
__u16 ac_ahz; /* AHZ */
#endif /* __KERNEL__ */

and Sam's 7 line patch is:
#ifndef __KERNEL__
__u16 ac_ahz; /* AHZ */
#else
#ifndef CONFIG_M68K
__u16 ac_ahz; /* AHZ */
#endif
#endif

Ingo, please apply the patch which one you like, I just want to get rid of this warning ;-)

Thanks,
--
JSR

2009-01-21 01:28:34

[permalink] [raw]

Subject: Re: [GIT PULL -tip] fix 41 'make headers_check' warnings

Hello Sam,

On Sun, 2009-01-18 at 12:02 +0100, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <[email protected]> wrote:
> > --- a/include/linux/types.h
> > +++ b/include/linux/types.h
> > @@ -138,6 +138,7 @@ typedef __s64 int64_t;
> > *
> > * blkcnt_t is the type of the inode's block count.
> > */
> > +#ifdef __KERNEL__
> > #ifdef CONFIG_LBD
> > typedef u64 sector_t;
> > typedef u64 blkcnt_t;
> > @@ -145,6 +146,10 @@ typedef u64 blkcnt_t;
> > typedef unsigned long sector_t;
> > typedef unsigned long blkcnt_t;
> > #endif
> > +#else /* __KERNEL__ */
> > +typedef unsigned long sector_t;
> > +typedef unsigned long blkcnt_t;
> > +#endif /* __KERNEL__ */
>

What is your patch for this case.

Thanks,
--
JSR

2009-01-21 05:52:25

[permalink] [raw]

Subject: Size of sector_t in userspace [Was: fix 41 'make headers_check' warnings]

Hi Jens.

In an attempt to fix some of the issues in our exported Headers jsr
encountered some strange stuff in types.h.

types.h is exported to userspace where we do not have access to CONFIG_*
symbols.
Despite this we use CONFGI_LBD to decide the size of sector_t like this:

#ifdef CONFIG_LBD
typedef u64 sector_t;
typedef u64 blkcnt_t;
#else
typedef unsigned long sector_t;
typedef unsigned long blkcnt_t;
#endif

But as CONFIG_LBD is never defined in userspace sector_t is now 32
bit on 32 bit boxes and 64 bit on 64 bit boxes (in userspace).

Is sector:t (and the companion blkcnt_t) really used by userspace?
If it is - what size is it expected to have?

Sam

2009-01-21 08:23:14

by Jens Axboe

[permalink] [raw]

Subject: Re: Size of sector_t in userspace [Was: fix 41 'make headers_check' warnings]

On Wed, Jan 21 2009, Sam Ravnborg wrote:
> Hi Jens.
>
> In an attempt to fix some of the issues in our exported Headers jsr
> encountered some strange stuff in types.h.
>
> types.h is exported to userspace where we do not have access to CONFIG_*
> symbols.
> Despite this we use CONFGI_LBD to decide the size of sector_t like this:
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> typedef u64 blkcnt_t;
> #else
> typedef unsigned long sector_t;
> typedef unsigned long blkcnt_t;
> #endif
>
> But as CONFIG_LBD is never defined in userspace sector_t is now 32
> bit on 32 bit boxes and 64 bit on 64 bit boxes (in userspace).
>
> Is sector:t (and the companion blkcnt_t) really used by userspace?
> If it is - what size is it expected to have?

I don't think it's used by userspace, at least it cannot be used in
userspace <-> kernelspace interactions, since the size isn't fixed.

--
Jens Axboe

2009-01-21 11:33:17