linux-kernel - Re: [PATCH 16/28] namespace: checkpatch wanking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111130065014.GU2203@ZenIV.linux.org.uk>
Date:	Wed, 30 Nov 2011 06:50:14 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Joe Perches <joe@...ches.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 16/28] namespace: checkpatch wanking

On Tue, Nov 29, 2011 at 07:39:11PM -0800, Joe Perches wrote:

> No worries.
> 
> I think patches 1 thru 14 are reasonable though
> and do apply with a few offsets to vfsmount-guts.

OK...  BTW, speaking of this one:

-static int mnt_id_start = 0;
+static int mnt_id_start;
 static int mnt_group_start = 1;

this deserves a separate NAK; explicit init here is actually more clear -
the variable is more or less analogous to mnt_group_start and it's less
distracting to have initializers spelled out in similar way as well.

-	if ((child_mnt = __lookup_mnt(path->mnt, path->dentry, 1)))
+	child_mnt = __lookup_mnt(path->mnt, path->dentry, 1);
+	if (child_mnt)
 		mntget(child_mnt);
 	br_read_unlock(&vfsmount_lock);
 	return child_mnt;

Maybe, maybe not; in the #vfsmount-guts I seriously pondered turning that
into
	child_mnt = ...;
	if (child_mnt) {
		mnt_add_count(child_mnt);
		br_read_unlock(...);
		return &child_mnt->mnt;
	} else {
		br_read_unlock(...);
		return NULL;
	}
Hell knows...  Since __lookup_mnt() returns struct mount now *and* lookup_mnt()
returns struct vfsmount (much shrunk subset of the original; almost nothing
in there is needed for anything outside of core VFS code) we need if or ?:
after unlock as well.  So it might make sense to split the codepaths once...

-	if (flags & MNT_FORCE && sb->s_op->umount_begin) {
+	if (flags & MNT_FORCE && sb->s_op->umount_begin)
 		sb->s_op->umount_begin(sb);
-	}

Probably.  It used to be just a method call, then it had grown lock/unlock
around it, then lock went down into the method instances and the compound
statement stayed.
 
-static int do_add_mount(struct vfsmount *newmnt, struct path *path, int mnt_flags)
+static int
+do_add_mount(struct vfsmount *newmnt, struct path *path, int mnt_flags)

I don't mind BSD style like that, but again, check #vfsmount-guts; this
one takes struct mount * now, so that line happens to be below 80 cols.

-static int select_submounts(struct vfsmount *parent, struct list_head *graveyard)
+static int
+select_submounts(struct vfsmount *parent, struct list_head *graveyard)

Ditto.

-		struct vfsmount *mnt = list_entry(tmp, struct vfsmount, mnt_child);
+		struct vfsmount *mnt = list_entry(tmp, struct vfsmount,
+						  mnt_child);

Ditto (mnt_child moved into struct mount now)
 
-	if (!(page = __get_free_page(GFP_KERNEL)))
+	page = __get_free_page(GFP_KERNEL);
+	if (!page)
 		return -ENOMEM;

Not sure...  In general I'd agree, but in this case...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/