Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 19:04 +0300, Alexey Dobriyan wrote:
> On Wed, Apr 27, 2011 at 5:52 PM, Ted Ts'o <tytso@xxxxxxx> wrote:
> > On Wed, Apr 27, 2011 at 04:47:40AM -0400, gmack@xxxxxxxxxxxxx wrote:
> >> Knowing about it and not screwing it up are two different things. I was
> >> working on a project a few years ago and we made this exact change thanks
> >> to the backwards logic of strcmp constantly screwing people up and the bug
> >> count went down considerably.
> >
> > If someone could even vaguely possibly screw up strcmp(), I don't want
> > them submitting patches to my subsystem.  I'm generally worried about
> > far more subtle bugs (deadlocks, locking screwups), and as Christoph
> > said, if you can't notice a strcmp bug, there's something
> > ***seriousl**** wrong with your code review process, test suite,
> > testing discpline, or all of the above.
> 
> This can be good excuse in math where proof of existence is enough.
> For some reason, the one making excuses always fails to say
> what exactly is wrong with all of the above.

Note, Ted only NAK'd it for his own code. Christoph seemed to do the
same for his. I'm fine with it in my code as when I see:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			strcmp (".", de->name) ||
			strcmp ("..", de1->name)) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}

(from fs/ext3/namei.c)

it does not stand out to me that this if statement will return true if
de->name does not equal "." or de1->name does not equal "..". If this
looked like:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			!streq (".", de->name) ||
			!streq ("..", de1->name)) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}

IMO looks much easier to understand.

Although I also think this is easier to understand too:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			strcmp (".", de->name) != 0 ||
			strcmp ("..", de1->name) != 0) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}


But again, I'm not the maintainer that needs to make sure this is
correct. I still feel this is personal preference decided by the
maintainer of said code.


> But instead of agreeing
> that, yes, streq() as "compare strings by value, relative order is not
> important,
> interesting or relevant" should have existed from the very beginning,
> let's add it into kernel at least,

I'm all for adding a streq() and may even use it. Simply because it
makes reading the code easier to understand, and not necessarily less
buggy (although I think it might help).

Note, I was having a similar discussion with a fellow kernel developer
on IRC a few years ago about using:

	!strcmp() vs strcmp() == 0

and in that discussion, the example the other developer had done with 
!strcmp() had a bug in it. Which did convince him to switch to
strcmp() == 0 ;)

-- Steve



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/