Re: [PATCH] fb: sm501fb off-by-1 sysfs store

Paul Mundt wrote:

On Thu, Mar 01, 2007 at 02:25:55PM +0100, Geert Uytterhoeven wrote:

quoted

On Thu, 1 Mar 2007, Paul Mundt wrote:

quoted

@@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	if (len < 1)
 		return -EINVAL;
 
-	if (strnicmp(buf, "crt", sizeof("crt")) == 0)
+	if (strnicmp(buf, "crt", 3) == 0)
 		head = HEAD_CRT;
-	else if (strnicmp(buf, "panel", sizeof("panel")) == 0)
+	else if (strnicmp(buf, "panel", 5) == 0)
 		head = HEAD_PANEL;
 	else
 		return -EINVAL;

What about using just `strlen("string")' instead? ISTR gcc optimizes this to a
constant. Not that it matters that much, as the actual string won't be
duplicated.

I stuck with the constants since they were already referenced in the
function, and there are enough other places in the kernel where they're
used for these things. I didn't figure that 5 was a terribly high number
to count to, but if there's expected difficulty, strlen() is of course an
option.

Personally I find it less visually offensive with constants, but as long
as the end number is the same, I'm not too concerned with how we get
there. Feel free to change it if you wish.

My personal preference is to use sizeof("string")-1:
- guaranteed to be optimized to a constant (better than strlen)
- less likely to be left alone when "string" becomes "a different string" 
(better than magic numbers)

I also prefer for repeated constants, either preprocessor macros (#define 
OPTION_CRT "crt") or constant char arrays (static const char Option_CRT[] = 
"crt"), or something like that, to ensure one copy of the string in the object. 
  But having the indirection here could add confusion for somebody trying to 
quickly determine what "crt" does.
-- 
Regards,
Paul Clarke

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV