linux-kernel - Re: [RFC][PATCH 5/14] Introduce union stack

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]

Date:	Mon, 14 May 2007 22:48:35 +0200 (MEST)
From:	Jan Engelhardt <jengelh@...ux01.gwdg.de>
To:	Bharata B Rao <bharata@...ux.vnet.ibm.com>
cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Jan Blunck <j.blunck@...harburg.de>
Subject: Re: [RFC][PATCH  5/14] Introduce union stack


On May 14 2007 15:10, Bharata B Rao wrote:
>+struct union_info * union_alloc(void)

Ultimate nitpick: try s/\* /*/; (also elsewhere)

>+static inline void union_lock(struct dentry *dentry)
>+{
>+	if (unlikely(dentry && dentry->d_union)) {
>+		struct union_info *ui = dentry->d_union;
>+
>+		UM_DEBUG_LOCK("\"%s\" locking %p (count=%d)\n",
>+			      dentry->d_name.name, ui,
>+			      atomic_read(&ui->u_count));
>+		__union_lock(dentry->d_union);
>+	}
>+}
>+
>+static inline void union_unlock(struct dentry *dentry)
>+{
>+	if (unlikely(dentry && dentry->d_union)) {
>+		struct union_info *ui = dentry->d_union;
>+
>+		UM_DEBUG_LOCK("\"%s\" unlocking %p (count=%d)\n",
>+			      dentry->d_name.name, ui,
>+			      atomic_read(&ui->u_count));
>+		__union_unlock(dentry->d_union);
>+	}
>+}

Do we really need the unlikely()? d_union may be a new feature,
but it may very well be possible that someone puts the bigger
part of his/her files under a union. And when d_unions get
stable, people will probably begin making their root filesystem
unioned for livecds, and then unlikely() will rather be a
likely penalty. My stance: just
	if (dentry != NULL && dentry->d_union != NULL)
This also goes for union_trylock.

>+static inline int union_trylock(struct dentry *dentry)
>+{
>+	int locked = 1;
>+
>+	if (unlikely(dentry && dentry->d_union)) {
>+		UM_DEBUG_LOCK("\"%s\" try locking %p (count=%d)\n",
>+			      dentry->d_name.name, dentry->d_union,
>+			      atomic_read(&dentry->d_union->u_count));
>+		BUG_ON(!atomic_read(&dentry->d_union->u_count));
>+		locked = mutex_trylock(&dentry->d_union->u_mutex);
>+		UM_DEBUG_LOCK("\"%s\" trylock %p %s\n", dentry->d_name.name,
>+			      dentry->d_union,
>+			      locked ? "succeeded" : "failed");
>+	}
>+	return (locked ? 1 : 0);
>+}

	return locked ? 1 : 0
or even
	return !!locked;
or since we're just passing up from mutex_trylock:
	return locked;
?

>+/*
>+ * This is a *I can't get no sleep* helper

More commonly known as "insomnia". :)


	Jan
-- 
-
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/