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