A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.
Signed-off-by: Roel Kluin <[email protected]>
---
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);
On Wed, 11 Nov 2009 22:26:35 +0100
Roel Kluin <[email protected]> wrote:
> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> 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?
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 <[email protected]>
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);
>
On Wed, 2009-11-11 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 11 Nov 2009 22:26:35 +0100
> Roel Kluin <[email protected]> wrote:
>
> > A negative error value is required: now we cannot
> > distinguish ENOMEM from a valid read of 12 bytes.
> >
> > Signed-off-by: Roel Kluin <[email protected]>
> > ---
> > 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 <[email protected]>
-- Steve
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 <[email protected]>
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 <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
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);
* Steven Rostedt <[email protected]> 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
Commit-ID: a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Gitweb: http://git.kernel.org/tip/a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author: Roel Kluin <[email protected]>
AuthorDate: Wed, 11 Nov 2009 22:26:35 +0100
Committer: Steven Rostedt <[email protected]>
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 <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
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);
>> ? ? ? 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
* Andy Whitcroft <[email protected]> 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
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
On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <[email protected]> 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);
On Wed, Nov 18, 2009 at 05:18:53AM -0500, Dan Merillat wrote:
> On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <[email protected]> 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