LinuxLists.cc - [PULL -tip] fixed few make headers

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

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

> Hello Ingo,
>
> Please pull these patches. Earlier I also floated these patches so that
> I can get some feedback.

Sam, Andrew, David, what's your workflow preference for these bits?

While they are oneliners and i could create a separate branch for these
and pull Jaswinder's tree (and do build coverage to make sure there's no
surprised on any arch), it's really up to the maintainers of these files
to decide on the workflow.

I'd lean towards doing this via the individual maintainers and/or -mm, but
no strong feelings ...

Even if it's done via a separate tree at minimum it needs the acks from
the affected maintainers.

Ingo

2009-01-13 21:04:05

by David Miller

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

From: Ingo Molnar <[email protected]>
Date: Tue, 13 Jan 2009 13:49:21 +0100

>
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > Hello Ingo,
> >
> > Please pull these patches. Earlier I also floated these patches so that
> > I can get some feedback.
>
> Sam, Andrew, David, what's your workflow preference for these bits?

I'm fine with these changes and you can take them in via your
tree.

2009-01-14 08:56:47

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Tue, 13 Jan 2009, Jaswinder Singh Rajput wrote:
> Please pull these patches. Earlier I also floated these patches so that I can get some feedback.
>
> The following changes since commit ae2659bcd0b5bf34dd4b03f1db0a2942cc09b8e6:
> Ingo Molnar (1):
> Merge branch 'x86/mm'
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master
>
> Jaswinder Singh Rajput (7):
> include of <linux/types.h> is preferred over <asm/types.h>
> found __[us]{8,16,32,64} type without #include <linux/types.h>
> capability.h: extern's make no sense in userspace
> coda_psdev.h: extern's make no sense in userspace
> in6.h: extern's make no sense in userspace
> nubus.h: extern's make no sense in userspace
> socket.h: extern's make no sense in userspace

May I please remind you of my comment in
http://patchwork.kernel.org/patch/2093/?

At least nubus.h still leaks forward declarations for kernel functions that
lack the "extern" keyword, so they still pollute user space.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2009-01-14 09:30:28

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

Hello Geert,

On Wed, Jan 14, 2009 at 2:26 PM, Geert Uytterhoeven
<[email protected]> wrote:
> On Tue, 13 Jan 2009, Jaswinder Singh Rajput wrote:
>> Please pull these patches. Earlier I also floated these patches so that I can get some feedback.
>>
>> The following changes since commit ae2659bcd0b5bf34dd4b03f1db0a2942cc09b8e6:
>> Ingo Molnar (1):
>> Merge branch 'x86/mm'
>>
>> are available in the git repository at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master
>>
>> Jaswinder Singh Rajput (7):
>> include of <linux/types.h> is preferred over <asm/types.h>
>> found __[us]{8,16,32,64} type without #include <linux/types.h>
>> capability.h: extern's make no sense in userspace
>> coda_psdev.h: extern's make no sense in userspace
>> in6.h: extern's make no sense in userspace
>> nubus.h: extern's make no sense in userspace
>> socket.h: extern's make no sense in userspace
>
> May I please remind you of my comment in
> http://patchwork.kernel.org/patch/2093/?
>

Thanks for your reminder :-)

> At least nubus.h still leaks forward declarations for kernel functions that
> lack the "extern" keyword, so they still pollute user space.
>

Yes, you are right, I just tried to pampered weeping warning messages
which is annoying.

In this subset, I just try to fix 77 such warnings.

nubus.h still have such warnings and other fixes required which you
are pointing so I am expecting nubus people will solve such problems.

Thanks
--
JSR

2009-01-14 15:36:55

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Wed, Jan 14, 2009 at 02:59:54PM +0530, Jaswinder Singh Rajput wrote:
> Hello Geert,
>
> On Wed, Jan 14, 2009 at 2:26 PM, Geert Uytterhoeven
> <[email protected]> wrote:
> > On Tue, 13 Jan 2009, Jaswinder Singh Rajput wrote:
> >> Please pull these patches. Earlier I also floated these patches so that I can get some feedback.
> >>
> >> The following changes since commit ae2659bcd0b5bf34dd4b03f1db0a2942cc09b8e6:
> >> Ingo Molnar (1):
> >> Merge branch 'x86/mm'
> >>
> >> are available in the git repository at:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master
> >>
> >> Jaswinder Singh Rajput (7):
> >> include of <linux/types.h> is preferred over <asm/types.h>
> >> found __[us]{8,16,32,64} type without #include <linux/types.h>
> >> capability.h: extern's make no sense in userspace
> >> coda_psdev.h: extern's make no sense in userspace
> >> in6.h: extern's make no sense in userspace
> >> nubus.h: extern's make no sense in userspace
> >> socket.h: extern's make no sense in userspace
> >
> > May I please remind you of my comment in
> > http://patchwork.kernel.org/patch/2093/?
> >
>
> Thanks for your reminder :-)
>
> > At least nubus.h still leaks forward declarations for kernel functions that
> > lack the "extern" keyword, so they still pollute user space.
> >
>
> Yes, you are right, I just tried to pampered weeping warning messages
> which is annoying.

Hi Jaswinder.

I appreciate your work but I will like to question the approach.
We should rather take the warnings as an indication that this
file needs to be looked over and fix not only the warnings
reported but rater to fix all the questionable issues on a file-by-file basis.
If the patch gets involved then it should be split in two.

Could I ask you to at least consider this approach.

Thanks,
Sam

2009-01-14 15:39:28

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Tue, Jan 13, 2009 at 01:49:21PM +0100, Ingo Molnar wrote:
>
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > Hello Ingo,
> >
> > Please pull these patches. Earlier I also floated these patches so that
> > I can get some feedback.
>
> Sam, Andrew, David, what's your workflow preference for these bits?
>
> While they are oneliners and i could create a separate branch for these
> and pull Jaswinder's tree (and do build coverage to make sure there's no
> surprised on any arch), it's really up to the maintainers of these files
> to decide on the workflow.
>
> I'd lean towards doing this via the individual maintainers and/or -mm, but
> no strong feelings ...

As they are one-liners for the most part I am fine with you handling
them in a eparate branch.

It is far better than I queue them up for at least two weeks before I
can give the kernel a bit of attention again.

Obviously it would be better if the Maintainers took them but history
tells us we cannot rely on that for all areas.

Sam

2009-01-14 15:59:13

by Oliver Hartkopp

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

Sam Ravnborg wrote:
> As they are one-liners for the most part I am fine with you handling
> them in a separate branch.
>

Hi Sam,

i would split them up:

One big single patch only for this

-#include <asm/types.h>
+#include <linux/types.h>

and this

+#include <linux/types.h>

which is trivial.

And all the other stuff should additionally go through the maintainers hands (at least).

Oliver

2009-01-14 16:00:13

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

Hello Sam,

On Wed, Jan 14, 2009 at 9:08 PM, Sam Ravnborg <[email protected]> wrote:
>> >

>
> I appreciate your work but I will like to question the approach.

My approach was:
"PATCH should solve a problem per file", like:
capability.h: extern's make no sense in userspace
coda_psdev.h: extern's make no sense in userspace
in6.h: extern's make no sense in userspace
nubus.h: extern's make no sense in userspace
socket.h: extern's make no sense in userspace

But this warnings was in many files:
include of <linux/types.h> is preferred over <asm/types.h> : 15 files
found __[us]{8,16,32,64} type without #include <linux/types.h> : 52 files

So in place of making 15 + 52 = 67 patches, I made 2 patches for each warning.

> We should rather take the warnings as an indication that this
> file needs to be looked over and fix not only the warnings
> reported but rater to fix all the questionable issues on a file-by-file basis.

Should I make 67 patches ?

--
JSR

2009-01-14 16:36:32

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Wed, 14 Jan 2009, Jaswinder Singh Rajput wrote:
> On Wed, Jan 14, 2009 at 9:08 PM, Sam Ravnborg <[email protected]> wrote:
> > I appreciate your work but I will like to question the approach.
>
> My approach was:
> "PATCH should solve a problem per file", like:
> capability.h: extern's make no sense in userspace
> coda_psdev.h: extern's make no sense in userspace
> in6.h: extern's make no sense in userspace
> nubus.h: extern's make no sense in userspace
> socket.h: extern's make no sense in userspace
>
> But this warnings was in many files:
> include of <linux/types.h> is preferred over <asm/types.h> : 15 files
> found __[us]{8,16,32,64} type without #include <linux/types.h> : 52 files
>
> So in place of making 15 + 52 = 67 patches, I made 2 patches for each warning.
>
> > We should rather take the warnings as an indication that this
> > file needs to be looked over and fix not only the warnings
> > reported but rater to fix all the questionable issues on a file-by-file basis.
>
> Should I make 67 patches ?

No.

What Sam means is that the warnings about externs not making sense in userspace
are indicators that there may be other external declarations (without "extern")
in those files, and that you should fix those at the same time (i.e. either
don't fix any of them, or fix all of them (in the same file)). If you don't
fix them at the same time, people tend to forget about them.

So the warnings are just considered canaries in our coal mine. Killing only the
canaries doesn't help.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2009-01-14 16:58:39

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Wed, Jan 14, 2009 at 10:06 PM, Geert Uytterhoeven
<[email protected]> wrote:
> What Sam means is that the warnings about externs not making sense in userspace
> are indicators that there may be other external declarations (without "extern")
> in those files, and that you should fix those at the same time (i.e. either
> don't fix any of them, or fix all of them (in the same file)). If you don't
> fix them at the same time, people tend to forget about them.
>
> So the warnings are just considered canaries in our coal mine. Killing only the
> canaries doesn't help.
>

If this is the case, then drop extern patches and let maintainers of
those files should handle them then they will not forget and never
repeat this mistake again.

--
JSR

2009-01-14 20:13:48

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Wed, Jan 14, 2009 at 05:36:14PM +0100, Geert Uytterhoeven wrote:
> On Wed, 14 Jan 2009, Jaswinder Singh Rajput wrote:
> > On Wed, Jan 14, 2009 at 9:08 PM, Sam Ravnborg <[email protected]> wrote:
> > > I appreciate your work but I will like to question the approach.
> >
> > My approach was:
> > "PATCH should solve a problem per file", like:
> > capability.h: extern's make no sense in userspace
> > coda_psdev.h: extern's make no sense in userspace
> > in6.h: extern's make no sense in userspace
> > nubus.h: extern's make no sense in userspace
> > socket.h: extern's make no sense in userspace
> >
> > But this warnings was in many files:
> > include of <linux/types.h> is preferred over <asm/types.h> : 15 files
> > found __[us]{8,16,32,64} type without #include <linux/types.h> : 52 files
> >
> > So in place of making 15 + 52 = 67 patches, I made 2 patches for each warning.
> >
> > > We should rather take the warnings as an indication that this
> > > file needs to be looked over and fix not only the warnings
> > > reported but rater to fix all the questionable issues on a file-by-file basis.
> >
> > Should I make 67 patches ?
>
> No.
>
> What Sam means is that the warnings about externs not making sense in userspace
> are indicators that there may be other external declarations (without "extern")
> in those files, and that you should fix those at the same time (i.e. either
> don't fix any of them, or fix all of them (in the same file)). If you don't
> fix them at the same time, people tend to forget about them.
>
> So the warnings are just considered canaries in our coal mine. Killing only the
> canaries doesn't help.

Correct. We should use the warnings to tell us that there may be something
fishy here and fix it all.
I've used checkpatch in the same way occasionally. If I had to do some
cleanup and checkpatch was a bit noisy I went through the file
_manually_. And when finished I ran chackpatch again and fixed up the
warnings/errors that made sense for me to fix and that I forgot when I refactored
the code.

So when I see "capability.h: extern's make no sense in userspace"
the perfect approach would be that the whole file was checked.

This is a much bigger task that just removing the warning.

(All this said I have sometimes doen the "remove warnings only trick too").

Sam

2009-01-15 10:56:18

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

* Sam Ravnborg <[email protected]> wrote:

> On Tue, Jan 13, 2009 at 01:49:21PM +0100, Ingo Molnar wrote:
> >
> > * Jaswinder Singh Rajput <[email protected]> wrote:
> >
> > > Hello Ingo,
> > >
> > > Please pull these patches. Earlier I also floated these patches so that
> > > I can get some feedback.
> >
> > Sam, Andrew, David, what's your workflow preference for these bits?
> >
> > While they are oneliners and i could create a separate branch for these
> > and pull Jaswinder's tree (and do build coverage to make sure there's no
> > surprised on any arch), it's really up to the maintainers of these files
> > to decide on the workflow.
> >
> > I'd lean towards doing this via the individual maintainers and/or -mm, but
> > no strong feelings ...
>
> As they are one-liners for the most part I am fine with you handling
> them in a eparate branch.
>
> It is far better than I queue them up for at least two weeks before I
> can give the kernel a bit of attention again.
>
> Obviously it would be better if the Maintainers took them but history
> tells us we cannot rely on that for all areas.

i dont think maintainers are bad at doing this - the problem and overhead
is on the submission side: the logistics of spreading dozens of very small
commits out to dozens of maintainers - each of who prefers a different
submission channel.

We have no automation for that (to make sure there's guaranteed progress
and to make sure there's no lost patch) and hence it's simply neither
efficient nor reliable to do it.

These particular commits seem rather uncontroversial - and they are not
part of some bigger facility so they are individually revertable as well.
Worst-case they break the build somewhere or cause a raised maintainer
eyebrow - we'll try to make sure that no such thing happens on a larger
scale.

Ingo

2009-01-15 11:12:36

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

* Sam Ravnborg <[email protected]> wrote:

> So when I see "capability.h: extern's make no sense in userspace" the
> perfect approach would be that the whole file was checked.
>
> This is a much bigger task that just removing the warning.

The problem is, you just littered the kernel with a ton of warnings and
declared that fixing them is a 'much bigger task'.

It's not good to start projects like this, because they simply never
finish. It's _much_ better to first reach a zero baseline via minimal
fixes, then enforce 'look at the whole thing' for all _NEW_ files and
changes.

That's how we are using checkpatch in the -tip topics, and it's working
very nicely.

Could we please start it like that?

Ingo

2009-01-15 14:35:51

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

On Thu, Jan 15, 2009 at 12:11:34PM +0100, Ingo Molnar wrote:
>
> * Sam Ravnborg <[email protected]> wrote:
>
> > So when I see "capability.h: extern's make no sense in userspace" the
> > perfect approach would be that the whole file was checked.
> >
> > This is a much bigger task that just removing the warning.
>
> The problem is, you just littered the kernel with a ton of warnings and
> declared that fixing them is a 'much bigger task'.
>
> It's not good to start projects like this, because they simply never
> finish. It's _much_ better to first reach a zero baseline via minimal
> fixes, then enforce 'look at the whole thing' for all _NEW_ files and
> changes.
>
> That's how we are using checkpatch in the -tip topics, and it's working
> very nicely.
>
> Could we please start it like that?

I would have done such have I had more time to do this stuff.
Real life just hit me after vacation finished and deadline approaching.

I can disable the warnings around -rc7 if we think they are too noisy
when we release the next kernel.

Sam

2009-01-15 14:48:30

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

Hello Sam,

On Thu, Jan 15, 2009 at 8:07 PM, Sam Ravnborg <[email protected]> wrote:
>
> I can disable the warnings around -rc7 if we think they are too noisy
> when we release the next kernel.
>

I will like to question the approach how you get this number '-rc7'

Do you think people can bear this noise till -rc7 or they will solve
all these warnings before -rc7

--
JSR

2009-01-15 14:50:29

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

* Sam Ravnborg <[email protected]> wrote:

> On Thu, Jan 15, 2009 at 12:11:34PM +0100, Ingo Molnar wrote:
> >
> > * Sam Ravnborg <[email protected]> wrote:
> >
> > > So when I see "capability.h: extern's make no sense in userspace" the
> > > perfect approach would be that the whole file was checked.
> > >
> > > This is a much bigger task that just removing the warning.
> >
> > The problem is, you just littered the kernel with a ton of warnings and
> > declared that fixing them is a 'much bigger task'.
> >
> > It's not good to start projects like this, because they simply never
> > finish. It's _much_ better to first reach a zero baseline via minimal
> > fixes, then enforce 'look at the whole thing' for all _NEW_ files and
> > changes.
> >
> > That's how we are using checkpatch in the -tip topics, and it's working
> > very nicely.
> >
> > Could we please start it like that?
>
> I would have done such have I had more time to do this stuff. Real life
> just hit me after vacation finished and deadline approaching.
>
> I can disable the warnings around -rc7 if we think they are too noisy
> when we release the next kernel.

they are perfectly fine i think, lets just not do the full approach now
and reach a zero baseline first. Then perfectionism can start again, at a
much more manageable flux.

Ingo

2009-01-15 14:51:26

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

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

> Hello Sam,
>
> On Thu, Jan 15, 2009 at 8:07 PM, Sam Ravnborg <[email protected]> wrote:
> >
> > I can disable the warnings around -rc7 if we think they are too noisy
> > when we release the next kernel.
>
> I will like to question the approach how you get this number '-rc7'
>
> Do you think people can bear this noise till -rc7 or they will solve all
> these warnings before -rc7

the noise is not bearable, it hides a lot of real bugs. We should do the
fixes you posted and get them to Linus.

Ingo

2009-01-15 14:52:24

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

* Ingo Molnar <[email protected]> wrote:

>
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > Hello Sam,
> >
> > On Thu, Jan 15, 2009 at 8:07 PM, Sam Ravnborg <[email protected]> wrote:
> > >
> > > I can disable the warnings around -rc7 if we think they are too noisy
> > > when we release the next kernel.
> >
> > I will like to question the approach how you get this number '-rc7'
> >
> > Do you think people can bear this noise till -rc7 or they will solve all
> > these warnings before -rc7
>
> the noise is not bearable, it hides a lot of real bugs. We should do the
> fixes you posted and get them to Linus.

btw., could you please put them into your Git tree and post a pull
request, so that i can check how many warnings there are left?

Ingo

2009-01-15 15:16:26

[permalink] [raw]

Subject: Re: [PULL -tip] fixed few make headers_check warnings

Hello Ingo,

On Thu, Jan 15, 2009 at 8:21 PM, Ingo Molnar <[email protected]> wrote:
>
> btw., could you please put them into your Git tree and post a pull
> request, so that i can check how many warnings there are left?
>

Patches are still in my git tree and I also cross checked that they
are mergeable with your latest -tip.

The following changes since commit ae2659bcd0b5bf34dd4b03f1db0a2942cc09b8e6:
Ingo Molnar (1):
Merge branch 'x86/mm'

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git
master

Jaswinder Singh Rajput (7):
include of <linux/types.h> is preferred over <asm/types.h>
found __[us]{8,16,32,64} type without #include <linux/types.h>
capability.h: extern's make no sense in userspace
coda_psdev.h: extern's make no sense in userspace
in6.h: extern's make no sense in userspace
nubus.h: extern's make no sense in userspace
socket.h: extern's make no sense in userspace

Thanks

--
JSR

2009-01-15 15:34:46