[PATCH] ftrace: return error instead of 12 bytes read

[PATCH] ftrace: return error instead of 12 bytes read

[Roel Kluin]

A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 
 	trace_seq_init(s);
 

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andrew Morton]

On Wed, 11 Nov 2009 22:26:35 +0100
Roel Kluin <roel.kluin@gmail.com> wrote:

> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b20d3ec..03c7fd5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
>  
>  	s = kmalloc(sizeof(*s), GFP_KERNEL);
>  	if (!s)
> -		return ENOMEM;
> +		return -ENOMEM;
>  
>  	trace_seq_init(s);
>  

lol, there we go again.

Andy, can we have a checkpatch rule please?

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Steven Rostedt]

On Wed, 2009-11-11 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 11 Nov 2009 22:26:35 +0100
> Roel Kluin <roel.kluin@gmail.com> wrote:
> 
> > A negative error value is required: now we cannot
> > distinguish ENOMEM from a valid read of 12 bytes.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> >  kernel/trace/trace.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index b20d3ec..03c7fd5 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
> >  
> >  	s = kmalloc(sizeof(*s), GFP_KERNEL);
> >  	if (!s)
> > -		return ENOMEM;
> > +		return -ENOMEM;
> >  
> >  	trace_seq_init(s);
> >  
> 
> lol, there we go again.
> 
> Andy, can we have a checkpatch rule please?

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Alexey Dobriyan]

On 11/12/09, Ingo Molnar <mingo@elte.hu> wrote:
>  drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird

"weirdness" comes from BSD where E codes are negative,
so they do not have this problem at all.

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andy Whitcroft]

>>       s = kmalloc(sizeof(*s), GFP_KERNEL);
>>       if (!s)
>> -             return ENOMEM;
>> +             return -ENOMEM;
>>
>>       trace_seq_init(s);
>>
>
> lol, there we go again.
>
> Andy, can we have a checkpatch rule please?

Thats a tricky one.  Not only do we not really have a sensible way to
know if ENOMEM is an errno, we also find a bunch of places that we
appear to use positive errno's as return values where we would falsly
complain about.  Its particularly common in scsi and filesystems.
Admittedly the vast majority are return -EXXX form, so we could add
this as a non-default check perhaps.

Thoughts?

-apw

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Ingo Molnar]

* Andy Whitcroft <apw@canonical.com> wrote:

> >> ? ? ? s = kmalloc(sizeof(*s), GFP_KERNEL);
> >> ? ? ? if (!s)
> >> - ? ? ? ? ? ? return ENOMEM;
> >> + ? ? ? ? ? ? return -ENOMEM;
> >>
> >> ? ? ? trace_seq_init(s);
> >>
> >
> > lol, there we go again.
> >
> > Andy, can we have a checkpatch rule please?
> 
> Thats a tricky one.  Not only do we not really have a sensible way to 
> know if ENOMEM is an errno, we also find a bunch of places that we 
> appear to use positive errno's as return values where we would falsly 
> complain about.  Its particularly common in scsi and filesystems. 
> Admittedly the vast majority are return -EXXX form, so we could add 
> this as a non-default check perhaps.
> 
> Thoughts?

Even in filesystems, ~80% of the cases use proper negative values:

 $ git grep 'return -E' fs/ | wc -l
 4540
 $ git grep 'return E' fs/ | wc -l
 895

For SCSI it's even better, ~97% of the cases use the kernel's standard:

 $ git grep 'return -E' drivers/scsi/ | wc -l
 1448
 $ git grep 'return E' drivers/scsi/ | wc -l
 50

So i'd suggest to make this a default-enabled check. (default disabled 
checks are used only by a small minority) For a _long_ time has this 
been the kernel standard.

	Ingo

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andy Whitcroft]

On Thu, Nov 12, 2009 at 02:45:54PM +0100, Ingo Molnar wrote:

> So i'd suggest to make this a default-enabled check. (default disabled 
> checks are used only by a small minority) For a _long_ time has this 
> been the kernel standard.

Andrew, I've put a dirty hack in to see how well it stands up against
your incoming flow.  This is in the version at the URL below (it may
take a bit to mirror out):

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

-apw

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Dan Merillat]

On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <mingo@elte.hu> wrote:

> Even in filesystems, ~80% of the cases use proper negative values:
>
>  $ git grep 'return -E' fs/ | wc -l
>  4540
>  $ git grep 'return E' fs/ | wc -l
>  895

Except....
fs/9p/fid.c:                            return ERR_PTR(-EPERM);

try this:

$ git grep "return E[A-Z]*;" | grep -v EOF | grep -v ERROR | wc -l
138
$ git grep "return -E[A-Z]*;"| wc -l
57285

2 of those are in Documentation/ and 2 are comments from a quick
glance.   134 uses of positive error returns, _74_ of which are in
fs/xfs, 24 in bluetooth, and the rest scattered randomly around the
kernel.

134 positive error returns  vs 57881 negative?  The style is so
strongly ingrained in the kernel (And I'd bet an audit of those
remaning 134 would find at least one bug) that it'd be a good janitor
task to go through and switch them.


This is one example - a function that returns either ENXIO or 0, and
the lone caller explicitly tests and flips the sign.

(Not signing off on this!!!!  but CCing the relevant people for this driver)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 6399e50..79867d6 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -428,7 +428,7 @@ cciss_proc_write(struct file *file, const char __user *buf,

                rc = cciss_engage_scsi(h->ctlr);
                if (rc != 0)
-                       err = -rc;
+                       err = rc;
                else
                        err = length;
        } else
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index 3315268..4af3085 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -1547,7 +1547,7 @@ cciss_engage_scsi(int ctlr)
        if (sa->registered) {
                printk("cciss%d: SCSI subsystem already engaged.\n", ctlr);
                spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
-               return ENXIO;
+               return -ENXIO;
        }
        sa->registered = 1;
        spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);

Re: [PATCH] ftrace: return error instead of 12 bytes read

[scameron]

On Wed, Nov 18, 2009 at 05:18:53AM -0500, Dan Merillat wrote:
> On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <mingo@elte.hu> wrote:
> 
> > Even in filesystems, ~80% of the cases use proper negative values:
> >
> >  $ git grep 'return -E' fs/ | wc -l
> >  4540
> >  $ git grep 'return E' fs/ | wc -l
> >  895
> 
> Except....
> fs/9p/fid.c:                            return ERR_PTR(-EPERM);
> 
> try this:
> 
> $ git grep "return E[A-Z]*;" | grep -v EOF | grep -v ERROR | wc -l
> 138
> $ git grep "return -E[A-Z]*;"| wc -l
> 57285
> 
> 2 of those are in Documentation/ and 2 are comments from a quick
> glance.   134 uses of positive error returns, _74_ of which are in
> fs/xfs, 24 in bluetooth, and the rest scattered randomly around the
> kernel.
> 
> 134 positive error returns  vs 57881 negative?  The style is so
> strongly ingrained in the kernel (And I'd bet an audit of those
> remaning 134 would find at least one bug) that it'd be a good janitor
> task to go through and switch them.
> 
> 
> This is one example - a function that returns either ENXIO or 0, and
> the lone caller explicitly tests and flips the sign.
> 
> (Not signing off on this!!!!  but CCing the relevant people for this driver)
> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 6399e50..79867d6 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -428,7 +428,7 @@ cciss_proc_write(struct file *file, const char __user *buf,
> 
>                 rc = cciss_engage_scsi(h->ctlr);
>                 if (rc != 0)
> -                       err = -rc;
> +                       err = rc;
>                 else
>                         err = length;
>         } else
> diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
> index 3315268..4af3085 100644
> --- a/drivers/block/cciss_scsi.c
> +++ b/drivers/block/cciss_scsi.c
> @@ -1547,7 +1547,7 @@ cciss_engage_scsi(int ctlr)
>         if (sa->registered) {
>                 printk("cciss%d: SCSI subsystem already engaged.\n", ctlr);
>                 spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
> -               return ENXIO;
> +               return -ENXIO;
>         }
>         sa->registered = 1;
>         spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);

I sent a patch to fix this already.

http://groups.google.com/group/linux.kernel/browse_thread/thread/40b43d35dff0f30a/0a073fc8014547e0?hl=en&lnk=gst&q=cciss+weird#0a073fc8014547e0

-- steve

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Steven Rostedt]

On Wed, 2009-11-11 at 22:26 +0100, Roel Kluin wrote:
> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

Thanks! I'll pull this in right away and get it out to Ingo.

-- Steve

> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b20d3ec..03c7fd5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
>  
>  	s = kmalloc(sizeof(*s), GFP_KERNEL);
>  	if (!s)
> -		return ENOMEM;
> +		return -ENOMEM;
>  
>  	trace_seq_init(s);
>  

[PATCH][GIT PULL][v2.6.32] tracing: Fix return value of tracing_stats_read()

[Steven Rostedt]

Ingo,

I know this is late in the rc's, but this is a very minor fix. Do you
think we can slip this in before 33.

Please pull the latest tip/tracing/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/urgent


Roel Kluin (1):
      tracing: Fix return value of tracing_stats_read()

----
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author: Roel Kluin <roel.kluin@gmail.com>
Date:   Wed Nov 11 22:26:35 2009 +0100

    tracing: Fix return value of tracing_stats_read()
    
    The function tracing_stats_read() mistakenly returns ENOMEM instead
    of the negative value -ENOMEM.
    
    Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
    LKML-Reference: <4AFB2C0B.50605@gmail.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 
 	trace_seq_init(s);
 

Re: [PATCH][GIT PULL][v2.6.32] tracing: Fix return value of tracing_stats_read()

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> I know this is late in the rc's, but this is a very minor fix. Do you
> think we can slip this in before 33.

Yeah, it's an obvious oneliner.

> Please pull the latest tip/tracing/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/tracing/urgent
> 
> 
> Roel Kluin (1):
>       tracing: Fix return value of tracing_stats_read()
> 
> ----
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Pulled, thanks!

	Ingo

[tip:tracing/urgent] tracing: Fix return value of tracing_stats_read()

[tip-bot for Roel Kluin]

Commit-ID:  a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Gitweb:     http://git.kernel.org/tip/a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author:     Roel Kluin <roel.kluin@gmail.com>
AuthorDate: Wed, 11 Nov 2009 22:26:35 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 11 Nov 2009 21:26:55 -0500

tracing: Fix return value of tracing_stats_read()

The function tracing_stats_read() mistakenly returns ENOMEM instead
of the negative value -ENOMEM.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
LKML-Reference: <4AFB2C0B.50605@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 
 	trace_seq_init(s);