Re: [PATCH 19/20] drivers/net/ethernet/marvell/skge.c: fix error return code

Julia Lawall
2012-10-05T05:22:03Z

On Thu, 4 Oct 2012, Joe Perches wrote:

> On Thu, 2012-10-04 at 14:54 -0400, David Miller wrote:
>> From: Peter Senna Tschudin <peter.senna@gmail.com>
>>> On Thu, Oct 4, 2012 at 8:23 PM, David Miller <davem@davemloft.net> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>