[U-Boot] [PATCH] cmd_fdt.c: fix parse of byte streams and strings
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; }
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@f00 interrupts < 33 2 34 2 36 2 > fdt p /ethernet@f00
ethernet@f00 { interrupts = <0x21 0x2 0x22 0x2 0x24 0x2>; }; -----------------------------------------------------------------------
If understand Andy's changes correctly, what use to be fdt set /ethernet@f00 interrupts "<33 2 34 2 36 2>" is now fdt set /ethernet@f00 interrupts < 33 2 34 2 36 2 >
and you carried this forward to handle byte streams: fdt set /ethernet@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@f00 interrupts [ 33 2 34 2 36 2 ] and fdt set /ethernet@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@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];@@ -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;}
On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
fdt set /ethernet@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@f00 interrupts this is a string
How do you set a string list, then?
-Scott
Scott Wood wrote:
On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
fdt set /ethernet@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@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@f00 interrupts "this is a string" (one parameter with explicit spaces) fdt set /ethernet@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@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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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
Jerry Van Baren wrote:
Scott Wood wrote:
On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
fdt set /ethernet@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@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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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
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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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
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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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@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
On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren gerald.vanbaren@ge.comwrote:
Scott Wood wrote:
On Thu, Sep 10, 2009 at 08:23:27PM -0400, Jerry Van Baren wrote:
fdt set /ethernet@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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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
Ken MacLeod wrote:
On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren <gerald.vanbaren@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@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@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
Ken MacLeod wrote:
On Fri, Sep 11, 2009 at 11:30 AM, Jerry Van Baren <gerald.vanbaren@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@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@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@f00 interrupts "[33 2 34 2 36 2]" becomes fdt set /ethernet@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@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)
participants (5)
-
Andy Fleming -
Jerry Van Baren -
Jerry Van Baren -
Ken MacLeod -
Scott Wood