On 06/03/16(Sun) 18:42, Patrick Wildt wrote:
> On Sun, Mar 06, 2016 at 04:23:08PM +0100, Martin Pieuchot wrote:
> > On 28/02/16(Sun) 17:49, Patrick Wildt wrote:
> > > Hi,
> > >
> > > If we're calling fdt_find_node() and do not actually find the node we're
> > > looking for, we call strncmp with a NULL value.
> > >
> > > What happens is that we use fdt_child_node(node) to retrieve a child
> > > and then use fdt_next_node(child) to go through the list of children.
> > > If we do not find a child that matches the given name(s), it will
> > > reach the end of the list. You know that you're at the end of the
> > > list if the next token is not FDT_NODE_BEGIN. A child must start
> > > with that token.
> > >
> > > Even though there's no child left, fdt_next_node(child) will not
> > > return a NULL ptr, but instead return a pointer to the next token.
> > > This means the for-loop will continue to run and call strncmp.
> > > fdt_node_name(child) will return a NULL ptr, as the token behind
> > > the ptr is not FDT_NODE_BEGIN.
> >
> > Is it possible to fix fdt_next_node() then?
>
> That's a good question and is probably the better fix. fdt_next_node()
> depends on recursion and expects itself to return a pointer to the
> next node, no matter if it exists or not. With a bit of code
> duplication this can be worked around. Basically by implementing
> another function that did what fdt_next_node() did before, and
> then have a check at the end of fdt_next_node() to only return
> the node if it exists.
Some comments below.
> @@ -171,8 +172,36 @@ fdt_node_property(void *node, char *name, char **out)
> }
>
> /*
> - * Retrieves next node, skipping all the children nodes of the pointed node
> - * if passed 0 wil return first node of the tree (root)
> + * Retrieves next node, skipping all the children nodes of the pointed node,
> + * returns pointer to next node, no matter if it exists or not.
> + */
> +void *
> +fdt_skip_node(void *node)
Can we call it skip_child() to be in pair with the other two skip_*
functions?
> +{
> + u_int32_t *ptr = node;
> +
> + if (!tree_inited || !node)
^^^^
Please compare pointers against NULL but I doubt you need this check
here. node must not be NULL.
> + return NULL;
> +
> + if (betoh32(*ptr) != FDT_NODE_BEGIN)
> + return NULL;
I doubt you really want to return NULL from this function, unless you
check for NULL in the loops.
> +
> + ptr++;
> +
> + ptr = skip_node_name(ptr);
> + ptr = skip_props(ptr);
> +
> + /* skip children */
> + while (betoh32(*ptr) == FDT_NODE_BEGIN)
> + ptr = fdt_skip_node(ptr);
> +
> + return (betoh32(*ptr) == FDT_NODE_END) ? (ptr + 1) : NULL;
> +}
> +
> +/*
> + * Retrieves next node, skipping all the children nodes of the pointed node,
> + * returns pointer to next node if it exists, otherwise returns NULL.
> + * If passed 0 will return first node of the tree (root).
> */
> void *
> fdt_next_node(void *node)
> @@ -199,9 +228,15 @@ fdt_next_node(void *node)
>
> /* skip children */
> while (betoh32(*ptr) == FDT_NODE_BEGIN)
> - ptr = fdt_next_node(ptr);
> + ptr = fdt_skip_node(ptr);
>
> - return (betoh32(*ptr) == FDT_NODE_END) ? (ptr + 1) : NULL;
> + if (betoh32(*ptr) != FDT_NODE_END)
> + return NULL;
> +
> + if (betoh32(*(ptr + 1)) != FDT_NODE_BEGIN)
> + return NULL;
> +
> + return (ptr + 1);
> }
