Re: any chance of 2.6.0-test*?

[Adam Kropelin <akropel1@rochester.rr.com>]

On Sun, Jan 12, 2003 at 04:27:30PM -0500, Rob Wilkens wrote:
> On Sun, 2003-01-12 at 16:15, Matti Aarnio wrote:
> > On Sun, Jan 12, 2003 at 02:34:54PM -0500, Rob Wilkens wrote:
> > > Linus,
> > > 
> > > I'm REALLY opposed to the use of the word "goto" in any code where it's
> > > not needed.  OF course, I'm a linux kernel newbie, so I'm in no position
> > > to comment
> > 
> >     fs/open.c  file    do_sys_truncate()  function.
> > 
> > Explain how you can do that cleanly, understandably, and without
> > code duplication, or ugly kludges  without using those goto ?
> > (And sticking to coding-style.)
> 
> I've only compiled (and haven't tested this code), but it should be much
> faster than the original code.  Why?  Because we're eliminating an extra
> "jump" in several places in the code every time open would be called. 

Congratulations. You've possibly increased the speed of an error path by 
an infintessimal amount at the expense of increasing the size of kernel
image and making the code harder to read and maintain. (I say "possibly"
since with caching effects you may have actually slowed the code down.)

Harder to read: The primary code path is polluted with repetative code
that has no bearing on its primary mission.

Harder to maintain: Add an extra resource allocation near the top and
now you have to hunt out every failure case and manually update them all
(with yet more duplicate code) instead of just amending the cleanup code
at the end of the function.

>  	error = -EINVAL;
>  	if (length < 0)	/* sorry, but loff_t says... */
> -		goto out;
> +		return error;

Possibly an improvement, though there are maintenance and consistency
arguments against it.

>  	/* For directories it's -EISDIR, for other non-regulars - -EINVAL */
>  	error = -EISDIR;
> -	if (S_ISDIR(inode->i_mode))
> -		goto dput_and_out;
> +	if (S_ISDIR(inode->i_mode)){
> +		path_release(&nd);
> +		return error;
> +	}

Nope. This code is precisely what the on-error-goto method is used
to avoid.

The use of goto in the kernel surprised me the first time I saw it, too.
However, rather than hurry to point out how much more I knew about C
style than the kernel hackers, I stayed quiet and made a learning
experience of it. I suggest you do the same.

--Adam