On Mon, Mar 07, 2016 at 01:25:01PM +0100, Martin Pieuchot wrote:
> 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?
I wouldn't call it skip_child(), as it skips to the next node on the
same level of hierarchy, so I'd rather call it skip_node().
Is this better?
diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
index c86df3e..1d0422b 100644
--- sys/dev/ofw/fdt.c
+++ sys/dev/ofw/fdt.c
@@ -30,6 +30,7 @@ char *fdt_get_str(u_int32_t);
void *skip_property(u_int32_t *);
void *skip_props(u_int32_t *);
void *skip_node_name(u_int32_t *);
+void *skip_node(void *);
void *fdt_parent_node_recurse(void *, void *);
#ifdef DEBUG
void fdt_print_node_recurse(void *, int);
@@ -172,8 +173,30 @@ 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 *
+skip_node(void *node)
+{
+ u_int32_t *ptr = node;
+
+ ptr++;
+
+ ptr = skip_node_name(ptr);
+ ptr = skip_props(ptr);
+
+ /* skip children */
+ while (betoh32(*ptr) == FDT_NODE_BEGIN)
+ ptr = skip_node(ptr);
+
+ return (ptr + 1);
+}
+
+/*
+ * Retrieves next node, skipping all the children nodes of the pointed node,
+ * returns pointer to next node if exists, otherwise returns NULL.
+ * If passed 0 will return first node of the tree (root).
*/
void *
fdt_next_node(void *node)
@@ -185,7 +208,7 @@ fdt_next_node(void *node)
ptr = node;
- if (!node) {
+ if (node == NULL) {
ptr = tree.tree;
return (betoh32(*ptr) == FDT_NODE_BEGIN) ? ptr : NULL;
}
@@ -200,9 +223,15 @@ fdt_next_node(void *node)
/* skip children */
while (betoh32(*ptr) == FDT_NODE_BEGIN)
- ptr = fdt_next_node(ptr);
+ ptr = skip_node(ptr);
+
+ if (betoh32(*ptr) != FDT_NODE_END)
+ return NULL;
+
+ if (betoh32(*(ptr + 1)) != FDT_NODE_BEGIN)
+ return NULL;
- return (betoh32(*ptr) == FDT_NODE_END) ? (ptr + 1) : NULL;
+ return (ptr + 1);
}
/*
>
>
> > +{
> > + 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);
> > }
>
