Wolfgang Grandegger wrote:
Hi Jerry,
Jerry Van Baren wrote: [...]
Hi Wolfgang G,
I've applied your patches to my local (working) repository and will push the changes tonight (my tonight, your tomorrow ;-). I created a subroutine out of three snippets of code in cmd_fdt.c which your fdt_find_node_by_path() change fixed up so I had to fix one line in the new subroutine by hand. Not bad at all considering the changes I made in that file.
Thanks. In the meantime I observed, that
fdt_find_node_by_path(fdt, 0, "/");
returns an error. Is that by purpose?
[snip]
Wolfgang.
Hmm, interesting observation. The character '/' is a figment of our imagination, offset 0 in the tree is the root node. The character '/' is the path separator and doesn't actually exist anywhere in the fdt - when parsing paths, the stuff between the slashes is searched for and the slashes themselves are skipped over.
What you asked in the above call is a node with no name under the root node, i.e. "//" in human-speak. That wasn't found, of course. On the other hand, it is a pretty obvious "mistake" (I was guilty of making the same mistake when I first tried to use fdt_path_offset()).
It seems like I was forever doing a conditional:
59 if (strcmp(pathp, "/") == 0) { 60 nodeoffset = 0; 61 } else { 62 nodeoffset = fdt_path_offset (fdt, pathp); 63 if (nodeoffset < 0) {
Trivia: Your patch we are discussing changed exactly this fdt_path_offset() into fdt_find_node_by_path() - this is the one place your patch didn't apply, because I refactored the three conditionals into a single wrapper subroutine.
At the risk of being accused of codling our users, I would propose we add the equivalent "/" detection code (above) to fdt_find_node_by_path(), (I will do that tonight unless you beat me to it). It seems silly to have the caller replicate or wrap the conditional since it is going to be such a common idiom/mistake.
Thanks, gvb