[PATCH] ftrace: return error instead of 12 bytes read
Roel Kluin
distinguish ENOMEM from a valid read of 12 bytes.
Signed-off-by: Roel Kluin <roel....@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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Andrew Morton
Roel Kluin <roel....@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....@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?
Steven Rostedt
> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
>
> Signed-off-by: Roel Kluin <roel....@gmail.com>
Thanks! I'll pull this in right away and get it out to Ingo.
-- Steve
Steven Rostedt
> On Wed, 11 Nov 2009 22:26:35 +0100
> Roel Kluin <roel....@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....@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 <ros...@goodmis.org>
-- Steve
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....@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....@gmail.com>
LKML-Reference: <4AFB2C0...@gmail.com>
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Ingo Molnar
* Steven Rostedt <ros...@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-bot for Roel Kluin
Gitweb: http://git.kernel.org/tip/a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author: Roel Kluin <roel....@gmail.com>
AuthorDate: Wed, 11 Nov 2009 22:26:35 +0100
Committer: Steven Rostedt <ros...@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....@gmail.com>
LKML-Reference: <4AFB2C0...@gmail.com>
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Alexey Dobriyan
> 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.
Andy Whitcroft
>> � � � 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
Ingo Molnar
* Andy Whitcroft <a...@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
Andy Whitcroft
> 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
Dan Merillat
> 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);
scam...@beardog.cce.hp.com
I sent a patch to fix this already.
-- steve