From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Sat, 30 Apr 2011 00:16:36 +0200
Subject: [U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for
	the L2 cache node
In-Reply-To: <4DBB2723.4050408@freescale.com>
References: <1304089126-11945-1-git-send-email-timur@freescale.com>
	<20110429203058.446B9D5270C@gemini.denx.de>
	<20110429154457.0fee37c6@schlenkerla.am.freescale.net>
	<20110429205513.382ABD5270C@gemini.denx.de>
	<4DBB2723.4050408@freescale.com>
Message-ID: <20110429221636.D08FFD5270C@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Timur Tabi,

In message <4DBB2723.4050408@freescale.com> you wrote:
>
> I disagree.  It's quite clear what I'm trying to do.  I'm trying to insert a

This is your opinion. I disagree.

> NULL character into a string.  Since device tree properties use a NULL to
> delimit multiple strings, it's clear that this is what the "0" is for.

Wrong data type. In C strings are _terminated_ by '\0' characters, so
using functions that are designed to deal with C strings are
obviously not the right tool to deal with data structures that have
_embedded_ NUL characters.

If you try, it quickly gets ugly like the code I rejected.

For example, who gives you any guarantee that sprintf() will continue
to append characters after it inserted the first NUL character?
A clever implementation could optimize this and return immediately
after seeing a NUL...

> Look at the original code:
> 
> 	len = sprintf(compat_buf,
> 		"fsl,%c%s-l2-cache-controller",
> 		tolower(cpu->name[0]), cpu->name + 1);
> 
> 	sprintf(&compat_buf[len + 1], "cache");
> 
> I think my patch is clearer than this.  In fact, because the original code was
> so obscure, there was a bug in it.  I could have done this:

Why exactly do you think you have to use sprintf() to append a
constant string like "cache"?

If you want to make clean what's intended, then use something like
this:

	len = sprintf(print_buf, "fsl,%c%s-l2-cache-controller",
		tolower(cpu->name[0]), cpu->name + 1);
	
	/* Include NUL characters */
	memcpy(compat_buf, print_buf, len + 1);
	memcpy(compat_buf + len + 1, "cache", sizeof("cache"));

If you want to optimize (I'm a fan of small memory footprint, but I'm
also a fan of readable code), use

	len = sprintf(compat_buf, "fsl,%c%s-l2-cache-controller",
		tolower(cpu->name[0]), cpu->name + 1);
	
	/* Include NUL characters */
	memcpy(compat_buf + len + 1, "cache", sizeof("cache"));

etc.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
egrep patterns are full regular expressions; it uses a fast  determi-
nistic algorithm that sometimes needs exponential space.
- unix manuals