Re: Common chmod and chown routines

[Elad Efrat <elad%NetBSD.org@localhost>]

Steven M. Bellovin wrote:
> On Mon, 20 Apr 2009 22:06:47 +0300
> Elad Efrat <elad%NetBSD.org@localhost> wrote:
>
> > I actually prefer this style, even if there is no point, and I am
> > going to cast a doubt over any claims of measurable performance
> > impact. :)
> The issue is the comprehensional complexity of goto.

I hold a different opinion, but seem to be at a minority at the moment.
Diff attached.

-e.

Index: vfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
retrieving revision 1.372
diff -u -p -r1.372 vfs_subr.c
--- vfs_subr.c  20 Apr 2009 18:06:26 -0000      1.372
+++ vfs_subr.c  20 Apr 2009 20:10:29 -0000
@@ -3233,22 +3233,18 @@ common_chmod_allowed(kauth_cred_t cred, 
        /* Superuser can always change mode. */
        error = kauth_authorize_generic(cred, KAUTH_GENERIC_ISSUSER,
            NULL);
-       if (!error) {
-               goto out;
-       }
+       if (!error)
+               return (0);
 
        /* Otherwise, user must own the file. */
-       if (kauth_cred_geteuid(cred) != cur_uid) {
-               goto out;
-       }
+       if (kauth_cred_geteuid(cred) != cur_uid)
+               return (EPERM);
 
        /*
         * Non-root users can't set the sticky bit on files.
         */
-       if ((vp->v_type != VDIR) && (new_mode & S_ISTXT)) {
-               error = EFTYPE;
-               goto out;
-       }
+       if ((vp->v_type != VDIR) && (new_mode & S_ISTXT))
+               return (EFTYPE);
 
        /*
         * If the invoker is trying to set the SGID bit on the file,
@@ -3259,19 +3255,11 @@ common_chmod_allowed(kauth_cred_t cred, 
 
                error = kauth_cred_ismember_gid(cred, cur_gid,
                    &ismember);
-               if (error)
-                       goto out;
-
-               if (!ismember) {
-                       error = EPERM;
-                       goto out;
-               }
+               if (error || !ismember)
+                       return (EPERM);
        }
 
-       error = 0;
-
- out:
-       return (error);
+       return (0);
 }
 
 /*
@@ -3301,9 +3289,8 @@ common_chown_allowed(kauth_cred_t cred, 
         */
        error = kauth_authorize_generic(cred, KAUTH_GENERIC_ISSUSER,
            NULL);
-       if (!error) {
-               goto out;
-       }
+       if (!error)
+               return (0);
 
        /*
         * You own the file and...
@@ -3312,25 +3299,20 @@ common_chown_allowed(kauth_cred_t cred, 
                /*
                 * You don't try to change ownership, and...
                 */
-                if (new_uid != cur_uid) {
-                       goto out;
-               }
+               if (new_uid != cur_uid)
+                       return (EPERM);
 
                /*
                 * You don't try to change group (no-op), or...
                 */
-               if (new_gid == cur_gid) {
-                       error = 0;
-                       goto out;
-               }
+               if (new_gid == cur_gid)
+                       return (0);
 
                /*
                 * Your effective gid is the new gid, or...
                 */
-               if (kauth_cred_getegid(cred) == new_gid) {
-                       error = 0;
-                       goto out;
-               }
+               if (kauth_cred_getegid(cred) == new_gid)
+                       return (0);
 
                /*
                 * The new gid is one you're a member of.
@@ -3338,15 +3320,10 @@ common_chown_allowed(kauth_cred_t cred, 
                ismember = 0;
                error = kauth_cred_ismember_gid(cred, new_gid,
                    &ismember);
-               if (error || !ismember) {
-                       error = EPERM;
-                       goto out;
-               }
-
-               error = 0;
+               if (error || !ismember)
+                       return (EPERM);
        }
 
- out:
-       return (error);
+       return (0);
 }