From: Peter Senna Tschudin <[email protected]>
Convert a nonnegative error return code to a negative one, as returned
elsewhere in the function.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <[email protected]>
---
drivers/net/ethernet/marvell/skge.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
index 5a30bf8..91836b5 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -3945,8 +3945,10 @@ static int __devinit skge_probe(struct pci_dev *pdev,
skge_board_name(hw), hw->chip_rev);
dev = skge_devinit(hw, 0, using_dac);
- if (!dev)
+ if (!dev) {
+ err = -ENOMEM;
goto err_out_led_off;
+ }
/* Some motherboards are broken and has zero in ROM. */
if (!is_valid_ether_addr(dev->dev_addr))
On Wed, 3 Oct 2012 18:18:10 +0200
Peter Senna Tschudin <[email protected]> wrote:
> From: Peter Senna Tschudin <[email protected]>
>
> Convert a nonnegative error return code to a negative one, as returned
> elsewhere in the function.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> (
> if@p1 (\(ret < 0\|ret != 0\))
> { ... return ret; }
> |
> ret@p1 = 0
> )
> ... when != ret = e1
> when != &ret
> *if(...)
> {
> ... when != ret = e2
> when forall
> return ret;
> }
> // </smpl>
>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
>
Thanks for looking into these kind of problems. The contents
of the patch are correct, but the automated commit message is useless.
You shouldn't just blindly say what the automated
script was looking for, you should describe what the bug is so that evaluators
can decide what the impact is and if it should be backported to stable
and vendor kernels.
Please resubmit the patchs with a reasonable analysis in the commit message.
Something like:
There is a bug in skge driver. If alloc_etherdev() fails, then
skge_devinit() will return NULL, and the skge_probe function incorrectly
returns success 0. It should return -ENOMEM instead.
From: Stephen Hemminger <[email protected]>
Date: Wed, 3 Oct 2012 09:25:08 -0700
> On Wed, 3 Oct 2012 18:18:10 +0200
> Peter Senna Tschudin <[email protected]> wrote:
>
>> From: Peter Senna Tschudin <[email protected]>
>>
>> Convert a nonnegative error return code to a negative one, as returned
>> elsewhere in the function.
>>
>> A simplified version of the semantic match that finds this problem is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> (
>> if@p1 (\(ret < 0\|ret != 0\))
>> { ... return ret; }
>> |
>> ret@p1 = 0
>> )
>> ... when != ret = e1
>> when != &ret
>> *if(...)
>> {
>> ... when != ret = e2
>> when forall
>> return ret;
>> }
>> // </smpl>
>>
>> Signed-off-by: Peter Senna Tschudin <[email protected]>
>>
>
> Thanks for looking into these kind of problems. The contents
> of the patch are correct, but the automated commit message is useless.
> You shouldn't just blindly say what the automated
> script was looking for, you should describe what the bug is so that evaluators
> can decide what the impact is and if it should be backported to stable
> and vendor kernels.
Agreed, I like seeing the checker script but I had that the entire
commit message is automated and has no human analysis or somments.
>
> Thanks for looking into these kind of problems. The contents
> of the patch are correct, but the automated commit message is useless.
> You shouldn't just blindly say what the automated
> script was looking for, you should describe what the bug is so that evaluators
> can decide what the impact is and if it should be backported to stable
> and vendor kernels.
>
> Please resubmit the patchs with a reasonable analysis in the commit message.
> Something like:
>
> There is a bug in skge driver. If alloc_etherdev() fails, then
> skge_devinit() will return NULL, and the skge_probe function incorrectly
> returns success 0. It should return -ENOMEM instead.
>
>
Stephen, I do not want to include function names on the commit
message. What do you think about this updated message, is it
acceptable?
--- --- ---
This patch fixes a bug related to the return value of the function. In some
error cases, the function return non-negative SUCCESS values, when the
correct would be a negative ERROR value.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
--- --- ---
Thanks,
Peter
--
Peter
On Thu, 4 Oct 2012 11:05:55 +0200
Peter Senna Tschudin <[email protected]> wrote:
> >
> > Thanks for looking into these kind of problems. The contents
> > of the patch are correct, but the automated commit message is useless.
> > You shouldn't just blindly say what the automated
> > script was looking for, you should describe what the bug is so that evaluators
> > can decide what the impact is and if it should be backported to stable
> > and vendor kernels.
> >
> > Please resubmit the patchs with a reasonable analysis in the commit message.
> > Something like:
> >
> > There is a bug in skge driver. If alloc_etherdev() fails, then
> > skge_devinit() will return NULL, and the skge_probe function incorrectly
> > returns success 0. It should return -ENOMEM instead.
> >
> >
>
> Stephen, I do not want to include function names on the commit
> message. What do you think about this updated message, is it
> acceptable?
>
No still to generic, it needs to be written by a human examining
the file and understanding what the cause and effect of the bug
is.
>> Stephen, I do not want to include function names on the commit
>> message. What do you think about this updated message, is it
>> acceptable?
>>
>
> No still to generic, it needs to be written by a human examining
> the file and understanding what the cause and effect of the bug
> is.
Stephen I've understood what you want. But it is not clear to me why
you want. Let me show what Coccinelle produces as output:
[peter@ace linux-next]$ spatch ../../cocci/ret4.cocci -dir .
...
* TODO [[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face1::linb=2894::colb=1::cole=3][./drivers/net/ethernet/sun/sungem.c::2894]]
[[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face2::linb=2966::colb=1::cole=3][./drivers/net/ethernet/sun/sungem.c::2966]]
[[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face2::linb=3015::colb=1::cole=7][./drivers/net/ethernet/sun/sungem.c::3015]]
...
There is "no" automatic code transformation. The semantic patch I'm
using only points out where to investigate to change, or not, the
code. The output is in Emcas org-mode format. So I can tell you that
the patches are not being robot generated. I'm making the patches, one
by one, with great help of Coccinelle, but I'm making the code changes
by hand.
I can't understand the advantages of describing each patch as you are
asking. "For me" the generic commit message together with the patch
makes sense. Can you please help me on that?
>
--
Peter
On Thu, 4 Oct 2012 19:32:12 +0200
Peter Senna Tschudin <[email protected]> wrote:
> >> Stephen, I do not want to include function names on the commit
> >> message. What do you think about this updated message, is it
> >> acceptable?
> >>
> >
> > No still to generic, it needs to be written by a human examining
> > the file and understanding what the cause and effect of the bug
> > is.
>
> Stephen I've understood what you want. But it is not clear to me why
> you want. Let me show what Coccinelle produces as output:
>
> [peter@ace linux-next]$ spatch ../../cocci/ret4.cocci -dir .
> ...
> * TODO [[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face1::linb=2894::colb=1::cole=3][./drivers/net/ethernet/sun/sungem.c::2894]]
> [[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face2::linb=2966::colb=1::cole=3][./drivers/net/ethernet/sun/sungem.c::2966]]
> [[view:./drivers/net/ethernet/sun/sungem.c::face=ovl-face2::linb=3015::colb=1::cole=7][./drivers/net/ethernet/sun/sungem.c::3015]]
> ...
>
> There is "no" automatic code transformation. The semantic patch I'm
> using only points out where to investigate to change, or not, the
> code. The output is in Emcas org-mode format. So I can tell you that
> the patches are not being robot generated. I'm making the patches, one
> by one, with great help of Coccinelle, but I'm making the code changes
> by hand.
>
> I can't understand the advantages of describing each patch as you are
> asking. "For me" the generic commit message together with the patch
> makes sense. Can you please help me on that?
The purpose of the commit message is not only so other developers understand
the patch. It is also so that the consumers (distro's and maintainers)
understand the scope of the impact. It maybe that your effort uncovers
a really bad security hole that requires a CVE and a re-release of a
major enterprise product like RHEL, or it could just be a minor corner
case that can never realistically happen. Unless you give a more complete
description, someone else will have to do it for each case.
Hello Stephen,
Thanks for the patience.
> The purpose of the commit message is not only so other developers understand
> the patch. It is also so that the consumers (distro's and maintainers)
> understand the scope of the impact. It maybe that your effort uncovers
I was not considering the scope of the impact. I'm fixing bugs found
by Coccinelle meaning that my bug hunting is based on static program
analysis. My evaluation of what to do is simpler: Can my change make
the code better? If I believe that yes, I make the patch.
> a really bad security hole that requires a CVE and a re-release of a
> major enterprise product like RHEL, or it could just be a minor corner
> case that can never realistically happen. Unless you give a more complete
> description, someone else will have to do it for each case.
This is a mix between what I do not want and what I can't do. I can't
give that complete description for consumers and distro maintainers
for drivers that I do not know completely. I do not know how critical
those bugs are in real life. I was expecting that the maintainer, who
knows the drivers details, would do that sort of things when needed.
Isn't enough fixing bugs and describing it correctly in a technical perspective?
Thanks,
Peter
--
Peter
From: Peter Senna Tschudin <[email protected]>
Date: Thu, 4 Oct 2012 19:32:12 +0200
> I can't understand the advantages of describing each patch as you are
> asking. "For me" the generic commit message together with the patch
> makes sense. Can you please help me on that?
Stop being so dense.
We want to know the implications of the bug being fixed.
Does it potentially cause an OOPS? Bad reference counting and thus
potential leaks or early frees?
You have to analyze the implications and ramifications of the bug
being fixed. We need that information.
Your commit messages are in fact robotic, they don't describe the
salient details of what kinds of problems the bug being fixed might
cause.
It's just "bad error code, this is the script that fixed it, kthx,
bye" which is pretty much useless for anaylsis.
On Thu, Oct 4, 2012 at 8:23 PM, David Miller <[email protected]> wrote:
> From: Peter Senna Tschudin <[email protected]>
> Date: Thu, 4 Oct 2012 19:32:12 +0200
>
>> I can't understand the advantages of describing each patch as you are
>> asking. "For me" the generic commit message together with the patch
>> makes sense. Can you please help me on that?
>
> Stop being so dense.
>
> We want to know the implications of the bug being fixed.
>
> Does it potentially cause an OOPS? Bad reference counting and thus
> potential leaks or early frees?
>
> You have to analyze the implications and ramifications of the bug
> being fixed. We need that information.
>
> Your commit messages are in fact robotic, they don't describe the
> salient details of what kinds of problems the bug being fixed might
> cause.
>
> It's just "bad error code, this is the script that fixed it, kthx,
> bye" which is pretty much useless for anaylsis.
Thank you David.
What about this as commit message?
--- // --
This patch fixes bug(s) related to return value of function(s). In
some error cases, the function is returning non-negative SUCCESS
value, when the correct would be negative ERROR value.
When on error, returning non negatives values, or SUCCESS, breaks error
propagation, producing unpredictable behavior that are very difficult
to debug.
--- // --
--
Peter
From: Peter Senna Tschudin <[email protected]>
Date: Thu, 4 Oct 2012 20:49:57 +0200
> On Thu, Oct 4, 2012 at 8:23 PM, David Miller <[email protected]> wrote:
>> From: Peter Senna Tschudin <[email protected]>
>> Date: Thu, 4 Oct 2012 19:32:12 +0200
>>
>>> I can't understand the advantages of describing each patch as you are
>>> asking. "For me" the generic commit message together with the patch
>>> makes sense. Can you please help me on that?
>>
>> Stop being so dense.
>>
>> We want to know the implications of the bug being fixed.
>>
>> Does it potentially cause an OOPS? Bad reference counting and thus
>> potential leaks or early frees?
>>
>> You have to analyze the implications and ramifications of the bug
>> being fixed. We need that information.
>>
>> Your commit messages are in fact robotic, they don't describe the
>> salient details of what kinds of problems the bug being fixed might
>> cause.
>>
>> It's just "bad error code, this is the script that fixed it, kthx,
>> bye" which is pretty much useless for anaylsis.
>
> Thank you David.
>
> What about this as commit message?
> --- // --
> This patch fixes bug(s) related to return value of function(s). In
> some error cases, the function is returning non-negative SUCCESS
> value, when the correct would be negative ERROR value.
>
> When on error, returning non negatives values, or SUCCESS, breaks error
> propagation, producing unpredictable behavior that are very difficult
> to debug.
> --- // --
What does it potentially cause the caller to do? Will it potentially
treat an error as a success and as a result register an object
illegally?
Real analysis please. The text you provided above is basically still
robotic and could be used to describe any error code return fix.
On Thu, 2012-10-04 at 14:54 -0400, David Miller wrote:
> From: Peter Senna Tschudin <[email protected]>
> > On Thu, Oct 4, 2012 at 8:23 PM, David Miller <[email protected]> wrote:
> >> We want to know the implications of the bug being fixed.
> >> Does it potentially cause an OOPS? Bad reference counting and thus
> >> potential leaks or early frees?
> >>
> >> You have to analyze the implications and ramifications of the bug
> >> being fixed. We need that information.
You are asking for deeper level analysis that may not
be reasonably possible from the robotic patch fixed
by a robotic piece of a bit of code correction via a
static analysis.
> >> It's just "bad error code, this is the script that fixed it, kthx,
> >> bye" which is pretty much useless for anaylsis.
Which may be all but impossible but for the handful of
folks that know all the gory/intimate details of the
original bit of code.
> What does it potentially cause the caller to do? Will it potentially
> treat an error as a success and as a result register an object
> illegally?
>
> Real analysis please. The text you provided above is basically still
> robotic and could be used to describe any error code return fix.
Right, it's useful to attempt but may be infeasible in
practice.
On Thu, 4 Oct 2012, Joe Perches wrote:
> On Thu, 2012-10-04 at 14:54 -0400, David Miller wrote:
>> From: Peter Senna Tschudin <[email protected]>
>>> On Thu, Oct 4, 2012 at 8:23 PM, David Miller <[email protected]> wrote:
>>>> We want to know the implications of the bug being fixed.
>>>> Does it potentially cause an OOPS? Bad reference counting and thus
>>>> potential leaks or early frees?
>>>>
>>>> You have to analyze the implications and ramifications of the bug
>>>> being fixed. We need that information.
>
> You are asking for deeper level analysis that may not
> be reasonably possible from the robotic patch fixed
> by a robotic piece of a bit of code correction via a
> static analysis.
As Peter pointed out, it is not even a robotic fix, if robotic means
literally that it was done by a robot. A tool was used to find a
potential problem, and then Peter studied the code to see what fix was
appropriate.
In this case, finding out the impact, requires looking up the call chain
in all directions to find out what the callers do with the returned value.
And understanding the likely impact along the way. Often the call chain
involves function pointers. This is all possible to do. And perhaps it
would be even more helpful to the consumers of the patch than just having
the problem fixed. But the fact remains that at other places in the
function, someone thought it was useful to return an error code, so in the
place where the error code return is missing, it seems like a useful fault
to fix, whether it has an impact now or might have one later. The main
human analysis required to generate the patch is to be convinced that the
given return path really represents an error condition and that the
function normally returns the specified kind of value in that case. If
there is some way in which these points are unclear, then the commit
message should certainly explain the reasoning that was used.
julia
>
>>>> It's just "bad error code, this is the script that fixed it, kthx,
>>>> bye" which is pretty much useless for anaylsis.
>
> Which may be all but impossible but for the handful of
> folks that know all the gory/intimate details of the
> original bit of code.
>
>> What does it potentially cause the caller to do? Will it potentially
>> treat an error as a success and as a result register an object
>> illegally?
>>
>> Real analysis please. The text you provided above is basically still
>> robotic and could be used to describe any error code return fix.
>
> Right, it's useful to attempt but may be infeasible in
> practice.
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Fri, 2012-10-05 at 07:22 +0200, Julia Lawall wrote:
> A tool was used to find a potential problem, and then Peter
> studied the code to see what fix was appropriate.
Hi Julia.
Was it true that a static analysis tool found the original
potential issue? If so, what tool was it?
But wasn't the scripted fix applied to the rest of the tree
robotically?
On Fri, Oct 05, 2012 at 12:36:35AM -0700, Joe Perches wrote:
> On Fri, 2012-10-05 at 07:22 +0200, Julia Lawall wrote:
> > A tool was used to find a potential problem, and then Peter
> > studied the code to see what fix was appropriate.
>
> Hi Julia.
>
> Was it true that a static analysis tool found the original
> potential issue? If so, what tool was it?
>
> But wasn't the scripted fix applied to the rest of the tree
> robotically?
It was coccinelle. It just prints a warning. You have to go in
manually, review the code and pick the correct error code.
regards,
dan carpenter
On Fri, 5 Oct 2012, Joe Perches wrote:
> On Fri, 2012-10-05 at 07:22 +0200, Julia Lawall wrote:
>> A tool was used to find a potential problem, and then Peter
>> studied the code to see what fix was appropriate.
>
> Hi Julia.
>
> Was it true that a static analysis tool found the original
> potential issue? If so, what tool was it?
In the very beginning, I think that I found the problem in a patch when
looking at patches that contain oopses.
>From that I wrote a Coccinelle rule. As Peter showed, the rule just
produces a list of line numbers. The fix cannot easily be automated,
because there are many cases where 0 is a valid error value. Some
functions, for example, have their error value as a nonpositive integer.
> But wasn't the scripted fix applied to the rest of the tree
> robotically?
No. Peter studied each case and considered what should be done, and then
did that. I guess a potentially bad fix could have been applied
automatically and then cleaned up manually, but considering the number of
cases where the fix would be wrong, that seem like a bad idea. Also one
might want to adapt a bit to local conventions about where the
initialization should be added.
julia
Stephen and David,
I've sent V2 of the patches and they were all accepted. Thank you.
I've made a template for the commit message, and then copy and paste
function names from the code. Something like:
-- // --
The function sky2_probe() return 0 for success and negative value
for most of its internal tests failures. There are two exceptions
that are error cases going to err_out*:. For this two cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
...
--//--
How useful it was to have the function names when you were analyzing
the patches? It took me a lot of time to modify the template by copy
and paste, check if it is correct, then commit. I have some other
similar patches to submit and I wonder if having the function names in
the commit message helped you.
Thank you,
Peter
On Fri, Oct 5, 2012 at 10:08 AM, Julia Lawall <[email protected]> wrote:
> On Fri, 5 Oct 2012, Joe Perches wrote:
>
>> On Fri, 2012-10-05 at 07:22 +0200, Julia Lawall wrote:
>>>
>>> A tool was used to find a potential problem, and then Peter
>>> studied the code to see what fix was appropriate.
>>
>>
>> Hi Julia.
>>
>> Was it true that a static analysis tool found the original
>> potential issue? If so, what tool was it?
>
>
> In the very beginning, I think that I found the problem in a patch when
> looking at patches that contain oopses.
>
> From that I wrote a Coccinelle rule. As Peter showed, the rule just
> produces a list of line numbers. The fix cannot easily be automated,
> because there are many cases where 0 is a valid error value. Some
> functions, for example, have their error value as a nonpositive integer.
>
>
>> But wasn't the scripted fix applied to the rest of the tree
>> robotically?
>
>
> No. Peter studied each case and considered what should be done, and then
> did that. I guess a potentially bad fix could have been applied
> automatically and then cleaned up manually, but considering the number of
> cases where the fix would be wrong, that seem like a bad idea. Also one
> might want to adapt a bit to local conventions about where the
> initialization should be added.
>
> julia
--
Peter
Hi Peter,
On Wed, Oct 10, 2012 at 2:08 PM, Peter Senna Tschudin
<[email protected]> wrote:
> Stephen and David,
>
> I've sent V2 of the patches and they were all accepted. Thank you.
>
> I've made a template for the commit message, and then copy and paste
> function names from the code. Something like:
>
> -- // --
> The function sky2_probe() return 0 for success and negative value
> for most of its internal tests failures. There are two exceptions
> that are error cases going to err_out*:. For this two cases, the
> function abort its success execution path, but returns non negative
> value, making it dificult for a caller function to notice the error.
>
> This patch fixes the error cases that do not return negative values.
>
> This was found by Coccinelle, but the code change was made by hand.
> This patch is not robot generated.
> ...
> --//--
>
> How useful it was to have the function names when you were analyzing
> the patches? It took me a lot of time to modify the template by copy
> and paste, check if it is correct, then commit. I have some other
> similar patches to submit and I wonder if having the function names in
> the commit message helped you.
>
Having real function names in your commit message won't make it more useful.
IMHO, the problem is you're still using a template commit message,
which produces
a robot-like commit message.
Developers don't like that, we prefer to see a text written by some
guy explaining
why is this patch needed, and what it's fixing/improving from an
overall point of view.
This is not easy and takes much training.
I believe patches should help maintainers, not only add work them,
so it's important to double-triple-check the patch and
double-triple-check the commit message.
I know this is tedious and it'll slow you a bit. But it's a good
thing: it means you are working :-)
Also, in this particular case, where the coccinelle does not fix
something obvious,
then I'd say you should be *extra* careful.
Hope this helps.
Ezequiel