[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Ken MacLeod]

Commit 4abd844d8e extended the fdt command parser to handle property
strings which are split across multiple arguments but it was broken for
byte streams and strings.  This patch fixes those.

Signed-off-by: Ken MacLeod <ken@bitsko.slc.ut.us>
---
 common/cmd_fdt.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
index 8683772..f0a8f0e 100644
--- a/common/cmd_fdt.c
+++ b/common/cmd_fdt.c
@@ -580,7 +580,7 @@ static int fdt_parse_prop(char **newval, int count, char *data, int *len)
 			*len    = *len + 1;
 			while (*newp == ' ')
 				newp++;
-			if (*newp != '\0')
+			if (*newp == '\0')
 				newp = newval[++stridx];
 		}
 		if (*newp != ']') {
@@ -593,10 +593,17 @@ static int fdt_parse_prop(char **newval, int count, char *data, int *len)
 		 * convenience (including the terminating '\0').
 		 */
 		while (stridx < count) {
-			*len = strlen(newp) + 1;
+			size_t length = strlen(newp);
 			strcpy(data, newp);
+			data += length;
+			*len += length;
 			newp = newval[++stridx];
+			if (stridx < count) {
+				*data++ = ' ';
+				*len += 1;
+			}
 		}
+		*len += 1;
 	}
 	return 0;
 }
-- 
1.5.4.7

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Jerry Van Baren]

Hi Ken,

Ken MacLeod wrote:
> Commit 4abd844d8e extended the fdt command parser to handle property
> strings which are split across multiple arguments but it was broken for
> byte streams and strings.  This patch fixes those.
> 
> Signed-off-by: Ken MacLeod <ken@bitsko.slc.ut.us>

Thanks for the patch.  Andy's patch 4abd844d8e says...
-----------------------------------------------------------------------
While I was in there, I extended the fdt command parser to handle 
property strings which are split across multiple arguments:

 > fdt set /ethernet at f00 interrupts < 33 2 34 2 36 2 >
 > fdt p /ethernet at f00
ethernet at f00 {
     interrupts = <0x21 0x2 0x22 0x2 0x24 0x2>;
};
-----------------------------------------------------------------------

If understand Andy's changes correctly, what use to be
   fdt set /ethernet at f00 interrupts "<33 2 34 2 36 2>"
is now
   fdt set /ethernet at f00 interrupts < 33 2 34 2 36 2 >

and you carried this forward to handle byte streams:
   fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
becomes
   fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
and
   fdt set /ethernet at f00 interrupts "this is a string"
can now handle multiple strings (words) by concatenating them with 
spaces (quoted strings still work the same as before because of hush's 
argument parsing)
   fdt set /ethernet at f00 interrupts this is a string

Best regards,
gvb

> ---
>  common/cmd_fdt.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
> index 8683772..f0a8f0e 100644
> --- a/common/cmd_fdt.c
> +++ b/common/cmd_fdt.c
> @@ -580,7 +580,7 @@ static int fdt_parse_prop(char **newval, int count, char *data, int *len)
>  			*len    = *len + 1;
>  			while (*newp == ' ')
>  				newp++;
> -			if (*newp != '\0')
> +			if (*newp == '\0')
>  				newp = newval[++stridx];
>  		}
>  		if (*newp != ']') {
> @@ -593,10 +593,17 @@ static int fdt_parse_prop(char **newval, int count, char *data, int *len)
>  		 * convenience (including the terminating '\0').
>  		 */
>  		while (stridx < count) {
> -			*len = strlen(newp) + 1;
> +			size_t length = strlen(newp);
>  			strcpy(data, newp);
> +			data += length;
> +			*len += length;
>  			newp = newval[++stridx];
> +			if (stridx < count) {
> +				*data++ = ' ';
> +				*len += 1;
> +			}
>  		}
> +		*len += 1;
>  	}
>  	return 0;
>  }

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Scott Wood]

On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
>    fdt set /ethernet at f00 interrupts "this is a string"
> can now handle multiple strings (words) by concatenating them with 
> spaces (quoted strings still work the same as before because of hush's 
> argument parsing)
>    fdt set /ethernet at f00 interrupts this is a string

How do you set a string list, then?

-Scott

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Jerry Van Baren]

Scott Wood wrote:
> On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
>>    fdt set /ethernet at f00 interrupts "this is a string"
>> can now handle multiple strings (words) by concatenating them with 
>> spaces (quoted strings still work the same as before because of hush's 
>> argument parsing)
>>    fdt set /ethernet at f00 interrupts this is a string
> 
> How do you set a string list, then?
> 
> -Scott

Hi Scott,

That *is* a string, the two examples are equivalent.  The proposed 
parser change glues all the parameters together with a single space. 
The string parsing doesn't worry me because it is 100% backward 
compatible with the original parsing, e.g. these two commands will 
result in the same string being stored in the FDT:
   fdt set /ethernet at f00 interrupts "this is a string"
(one parameter with explicit spaces)
   fdt set /ethernet at f00 interrupts this is a string
(five parameters with implicit spaces).

If you want two spaces between words, you would have to use the 
explicitly quoted version:
   fdt set /ethernet at f00 interrupts "this  is  a  string"

Strings are backwards compatible because the hush parser strips the 
quotes so all that that part of Ken's patch does is to extend it to 
paste together multiple arguments rather than limiting it to exactly one 
argument.  The following also produces the original string:
   fdt set /ethernet at f00 interrupts "this is" "a string"

I'm more concerned with the [] form because that really is a syntax 
change.  The original syntax with a single quoted argument will no 
longer be parsed if I understand the change (I need to apply the patch 
and confirm this):
Old:
    fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
becomes
    fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
Note that the *must* be a space between "[" and "33" and between "2" and 
"]" because the "[" and "]" now have to be separate arguments.  This is 
what Andy did with "<" and ">" with no public outcry, so it is probably OK.

--------------------------------------------------------------
==== Does anybody have a problem with this syntax change? ====
--------------------------------------------------------------

If humans are typing the commands it shouldn't be a big deal.  If 
someone has those commands embedded in a script, their script will break 
and they will have to update it per the new syntax.

FWIIW, Andy's syntax is much cleaner than my original (must quote) 
syntax (other than the need for spaces between the "[" "]" "<" ">" 
symbols and the numbers, but I can accept that).

Best regards,
gvb

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Scott Wood]

Jerry Van Baren wrote:
> Scott Wood wrote:
>> On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
>>>    fdt set /ethernet at f00 interrupts "this is a string"
>>> can now handle multiple strings (words) by concatenating them with 
>>> spaces (quoted strings still work the same as before because of 
>>> hush's argument parsing)
>>>    fdt set /ethernet at f00 interrupts this is a string
>>
>> How do you set a string list, then?
> 
> That *is* a string, 

No, a string list is not a string.  It is a set of strings that have been 
concatenated, but which retain their individual null terminators (as in the 
compatible property).

> Strings are backwards compatible because the hush parser strips the 
> quotes so all that that part of Ken's patch does is to extend it to 
> paste together multiple arguments rather than limiting it to exactly one 
> argument.  The following also produces the original string:
>   fdt set /ethernet at f00 interrupts "this is" "a string"
> 
> I'm more concerned with the [] form because that really is a syntax 
> change.  The original syntax with a single quoted argument will no 
> longer be parsed if I understand the change (I need to apply the patch 
> and confirm this):
> Old:
>    fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
> becomes
>    fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
> Note that the *must* be a space between "[" and "33" and between "2" and 
> "]" because the "[" and "]" now have to be separate arguments.  This is 
> what Andy did with "<" and ">" with no public outcry, so it is probably OK.
> 
> --------------------------------------------------------------
> ==== Does anybody have a problem with this syntax change? ====
> --------------------------------------------------------------

I'm not thrilled with it...  I'd think the code could be made to handle the [, 
], <, or > being in the same parameter as one of the numbers.

-Scott

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Andy Fleming]

>
>> Strings are backwards compatible because the hush parser strips the  
>> quotes so all that that part of Ken's patch does is to extend it to  
>> paste together multiple arguments rather than limiting it to  
>> exactly one argument.  The following also produces the original  
>> string:
>>  fdt set /ethernet at f00 interrupts "this is" "a string"
>> I'm more concerned with the [] form because that really is a syntax  
>> change.  The original syntax with a single quoted argument will no  
>> longer be parsed if I understand the change (I need to apply the  
>> patch and confirm this):
>> Old:
>>   fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
>> becomes
>>   fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
>> Note that the *must* be a space between "[" and "33" and between  
>> "2" and "]" because the "[" and "]" now have to be separate  
>> arguments.  This is what Andy did with "<" and ">" with no public  
>> outcry, so it is probably OK.
>> --------------------------------------------------------------
>> ==== Does anybody have a problem with this syntax change? ====
>> --------------------------------------------------------------
>
> I'm not thrilled with it...  I'd think the code could be made to  
> handle the [, ], <, or > being in the same parameter as one of the  
> numbers.

I agree.  Also, I don't think the capability of creating string  
parameters without using quotes is necessary, or necessarily a good  
idea.  The goal should be to make the values we pass in reflect the  
syntax of the device tree, itself, as that is what the naive observer  
would attempt.  If I required spaces around "<" and ">", then shame on  
me!  :)

Andy

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Jerry Van Baren]

Andy Fleming wrote:
>>
>>> Strings are backwards compatible because the hush parser strips the 
>>> quotes so all that that part of Ken's patch does is to extend it to 
>>> paste together multiple arguments rather than limiting it to exactly 
>>> one argument.  The following also produces the original string:
>>>  fdt set /ethernet at f00 interrupts "this is" "a string"
>>> I'm more concerned with the [] form because that really is a syntax 
>>> change.  The original syntax with a single quoted argument will no 
>>> longer be parsed if I understand the change (I need to apply the 
>>> patch and confirm this):
>>> Old:
>>>   fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
>>> becomes
>>>   fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
>>> Note that the *must* be a space between "[" and "33" and between "2" 
>>> and "]" because the "[" and "]" now have to be separate arguments. 
>>>  This is what Andy did with "<" and ">" with no public outcry, so it 
>>> is probably OK.
>>> --------------------------------------------------------------
>>> ==== Does anybody have a problem with this syntax change? ====
>>> --------------------------------------------------------------
>>
>> I'm not thrilled with it...  I'd think the code could be made to 
>> handle the [, ], <, or > being in the same parameter as one of the 
>> numbers.
> 
> I agree.  Also, I don't think the capability of creating string 
> parameters without using quotes is necessary, or necessarily a good 
> idea.  The goal should be to make the values we pass in reflect the 
> syntax of the device tree, itself, as that is what the naive observer 
> would attempt.  If I required spaces around "<" and ">", then shame on 
> me!  :)
> 
> Andy

I need to try
   fdt set /ethernet at f00 interrupts <33 2 34 2 36 2>
to confirm my reading of the code, it could be a limitation in my mental 
parsing, not the parsing code.

...and shame on me too if that is the case since I accepted your change. 
;-/  If true, fixing it shouldn't be difficult.

I really need to make some time to create automated regression tests. :-/

Best regards,
gvb

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Ken MacLeod]

On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren <gerald.vanbaren@ge.com>wrote:

> Scott Wood wrote:
> > On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
> >>    fdt set /ethernet at f00 interrupts "this is a string"
> >> can now handle multiple strings (words) by concatenating them with
> >> spaces (quoted strings still work the same as before because of hush's
> >> argument parsing)
> >>    fdt set /ethernet at f00 interrupts this is a string
> >
> > How do you set a string list, then?
>

The original code did not support string lists and this patch does not
address string lists.


> I'm more concerned with the [] form because that really is a syntax
> change.  The original syntax with a single quoted argument will no
> longer be parsed if I understand the change (I need to apply the patch
> and confirm this):
> Old:
>     fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
> becomes
>     fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
> Note that the *must* be a space between "[" and "33" and between "2" and
> "]" because the "[" and "]" now have to be separate arguments.  This is
> what Andy did with "<" and ">" with no public outcry, so it is probably OK.
>
> --------------------------------------------------------------
> ==== Does anybody have a problem with this syntax change? ====
> --------------------------------------------------------------
>
>
There is no change in syntax as far as I can tell, it should parse byte
strings the same as one argument with spaces or as multiple arguments.
There remains a side effect (bug?) that if the the '[' and the next value
are separate arguments, a 0x00 gets inserted into the data.  The original
code either didn't parse the complete byte list (incrementing stridx early)
or hung in an endless loop.  This patch fixes that case.

The fix on strings is that in the original code if there were multiple
arguments then only the last argument was stored, at least now it stores
multiple arguments (collapsing inter-argument space, if any).

I don't have Hush enabled right now and the non-Hush quoting rules are still
a little fuzzy for me right now.  The 'fdt set' wasn't working at all for me
without this patch.

  -- Ken

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Scott Wood]

Ken MacLeod wrote:
> On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren 
> <gerald.vanbaren at ge.com <mailto:gerald.vanbaren@ge.com>> wrote:
> 
>     Scott Wood wrote:
>      > On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
>      >>    fdt set /ethernet at f00 interrupts "this is a string"
>      >> can now handle multiple strings (words) by concatenating them with
>      >> spaces (quoted strings still work the same as before because of
>     hush's
>      >> argument parsing)
>      >>    fdt set /ethernet at f00 interrupts this is a string
>      >
>      > How do you set a string list, then?
> 
> 
> The original code did not support string lists and this patch does not 
> address string lists.

This patch takes the most obvious syntax for supporting a string list in the 
future, and makes it mean something else.

-Scott

[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings

[Jerry Van Baren]

Ken MacLeod wrote:
> On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren 
> <gerald.vanbaren at ge.com <mailto:gerald.vanbaren@ge.com>> wrote:
> 
>     Scott Wood wrote:
>      > On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
>      >>    fdt set /ethernet at f00 interrupts "this is a string"
>      >> can now handle multiple strings (words) by concatenating them with
>      >> spaces (quoted strings still work the same as before because of
>     hush's
>      >> argument parsing)
>      >>    fdt set /ethernet at f00 interrupts this is a string
>      >
>      > How do you set a string list, then?
> 
> The original code did not support string lists and this patch does not 
> address string lists.

The limitations of my original parsing shouldn't be taken as valid. ;-)

Scott has a very good point: the string parsing should take multiple 
(quoted) strings and turn them into proper string lists.  This is useful 
for things like compatibility lists.

>     I'm more concerned with the [] form because that really is a syntax
>     change.  The original syntax with a single quoted argument will no
>     longer be parsed if I understand the change (I need to apply the patch
>     and confirm this):
>     Old:
>        fdt set /ethernet at f00 interrupts "[33 2 34 2 36 2]"
>     becomes
>        fdt set /ethernet at f00 interrupts [ 33 2 34 2 36 2 ]
>     Note that the *must* be a space between "[" and "33" and between "2" and
>     "]" because the "[" and "]" now have to be separate arguments.  This is
>     what Andy did with "<" and ">" with no public outcry, so it is
>     probably OK.
> 
>     --------------------------------------------------------------
>     ==== Does anybody have a problem with this syntax change? ====
>     --------------------------------------------------------------
> 
> 
> There is no change in syntax as far as I can tell, it should parse byte 
> strings the same as one argument with spaces or as multiple arguments.  
> There remains a side effect (bug?) that if the the '[' and the next 

Bug. :-(

> value are separate arguments, a 0x00 gets inserted into the data.  The 
> original code either didn't parse the complete byte list (incrementing 
> stridx early) or hung in an endless loop.  This patch fixes that case.

OK, you made it better.  :-)

> The fix on strings is that in the original code if there were multiple 
> arguments then only the last argument was stored, at least now it stores 
> multiple arguments (collapsing inter-argument space, if any).

I agree with Scott, this is not good.  We should create lists.  If a 
user types
   fdt set /ethernet at f00 interrupts this is a string
he probably won't get what he wanted, but he will get what he deserves[1]:
   "this","is","a","string"
i.e. a list of four strings.

> I don't have Hush enabled right now and the non-Hush quoting rules are 
> still a little fuzzy for me right now.  The 'fdt set' wasn't working at 
> all for me without this patch.

I have not run with non-Hush in quite a while.  Sounds like I need to. 
The quoting rules should be the same.  Ahhh, you need to quote with 
apostrophes ' not double-quotes ".
   <http://www.denx.de/wiki/DULG/CommandLineParsing>
I don't know if spaces can be escaped with back-slashes off-hand, the 
page is silent on that.

>   -- Ken

Thanks,
gvb

[1]<http://www.lyricsdomain.com/18/rolling_stones/you_cant_always_get_what_you_want.html>

And I went down to the corporation
To get my fair share of abuse
Singing, "We're gonna vent our frustration
If we don't we're gonna blow a 50-amp fuse"
Sing it to me now...

You can't always get what you want
You can't always get what you want
You can't always get what you want
But if you try sometimes well you just might find
You get what you need

(yes, I changed one word)