From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 26 Apr 2008 08:40:07 +0200

quoted text
> On Sat, Apr 26, 2008 at 08:19:31AM +0200, Wolfgang Grandegger wrote:
> > What about removing the assignment "err =" in that case.
> Preferred.
> 
> See sample patch below (made on top of -linus
> so it does likely not apply but made it only to
> see the difference anyway).

I want to make one comment, directed at Wolfgang.

You are absolutely wrong, Wolfgang, in saying that Pavel's
original patch isn't easier to review than just changing
this code to go:

	if (copy_*_user())
		return -EFAULT;

In fact, recoding things like this is an immense extra hardship on a
reviewer.  I'll explain why.

If I see a patch that changes:

	err = SOMETHING;
	break;

into:

	err = SOMETHING_ELSE;
	break;

I know, WITH JUST READING THE PATCH, exactly what the side effects of
this change are.

I DO NOT need to bring the code into my editor and validate side
effects to the surrounding code.

I know that the assignment to 'err' is being changed, and that's it.

Whereas if you change:

	err = SOMETHING;
	break;

into:

	if (SOMETHING)
		return -SOME_ERROR;
	break;

I now have to bring the code into an editor and make sure that the
control flow change doesn't break things.

For example, maybe the exit of the switch statement was important, to
make sure cleanup code runs at the end of the function to release
locks, free allocated memory, etc.

With Pavel's patch it is not necessary to make such validations so
it's INFINITELY easier to validate.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html