Yeah, but..
My changes won't actually be incorporated (i hope) into the kernel.
I've since learned that a goto will optimize to be just as quick as what
I wrote (by studying the optimized assembler output at other
suggestion). Therefore, it was silly and pointless for me to spend the
10-12 minutes I did changing the code and testing it and e-mailing back
the changes.
So, there's nothing to worry about. Goto's are here forever.
-Rob
On Mon, 2003-01-13 at 06:08, Bernd Petrovitsch wrote:
> Rob Wilkens <robw@optonline.net> wrote:
> [...]
> >Here's the patch if you want to apply it (i have only compile tested it,
> >I haven't booted with it).. This patch applied to the 2.5.56 kernel.
> >
> >--- open.c.orig 2003-01-12 16:17:01.000000000 -0500
> >+++ open.c 2003-01-12 16:22:32.000000000 -0500
> >@@ -100,44 +100,58 @@
> >
> > error = -EINVAL;
> > if (length < 0) /* sorry, but loff_t says... */
> >- goto out;
> >+ return error;
> >
> > error = user_path_walk(path, &nd);
> > if (error)
> >- goto out;
> >+ return error;
> > inode = nd.dentry->d_inode;
> [ snipped the rest ]
>
> You just copied the logic to "cleanup and leave" the function several
> times. The (current, next and subsequent) maintainers at the next
> change in that function simply _have_ to check all cases as if they
> are different.
> Yes, _now_ you (and all others) know that they are identical. But in 6
> month after tons of patches?
>
> Perhaps you want to avoid goto's with:
> ---- snip (yes, it is purposely not a `diff -urN`) ----
> switch(0==0) {
> default:
> error = -EINVAL;
> if (length < 0) /* sorry, but loff_t says... */
> break;
> error = user_path_walk(path, &nd);
> if (error)
> break;
> inode = nd.dentry->d_inode;
>
> switch(0==0) {
> default:
> /* For directories it's -EISDIR, for other non-regulars - -EINVAL */
> error = -EISDIR;
> if (S_ISDIR(inode->i_mode))
> break;
> error = -EINVAL;
> if (!S_ISREG(inode->i_mode))
> break;
>
> error = permission(inode,MAY_WRITE);
> if (error)
> break;
>
> error = -EROFS;
> if(IS_RDONLY(inode))
> break;
>
> /* the rest omitted - the pattern should be clear */
>
> put_write_access(inode);
> break;
> }
> path_release(&nd);
> break;
> }
> return error;
> ---- snip ----
>
> FYI, backward goto's can be rewritten with:
> ---- snip ----
>
> for(;;) {
> <do something>
> if(i_want_to_go_back)
> continue;
> <do something_else>
> break;
> }
> ---- snip ----
>
> Are these two more understandable because the avoid the 'goto'?
> And try to see it not from the viewpoint of a first time reader, but
> from the viewpoint of a/the maintainer/developer (who reads this code
> probably quite often)?
>
> Bernd
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Jan 15 2003 - 22:00:45 EST