* [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
@ 2008-01-14 16:30 Scott Wood
2008-01-14 22:31 ` Stephen Rothwell
2008-01-15 0:16 ` David Gibson
0 siblings, 2 replies; 8+ messages in thread
From: Scott Wood @ 2008-01-14 16:30 UTC (permalink / raw)
To: jdl; +Cc: linuxppc-dev
ft_get_next_node() enumerates children of a given node.
ft_get_next_prop() enumerates propreties of a given node.
ft_getprop_offset() is like ft_getprop(), but takes a property offset rather
than a node offset and property name; it is primarily intended for use
with ft_get_next_prop().
Signed-off-by: Scott Wood <scottwood@freescale.com>
---
libfdt/fdt_ro.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++
libfdt/fdt_strerror.c | 1 +
libfdt/libfdt.h | 123 ++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 251 insertions(+), 7 deletions(-)
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 12a37d5..27c943a 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -293,6 +293,51 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
return prop->data;
}
+const void *fdt_getprop_offset(const void *fdt, int propoffset,
+ const char **name, int *lenp)
+{
+ uint32_t tag;
+ const struct fdt_property *prop;
+ int namestroff;
+ int err, len;
+
+ err = fdt_check_header(fdt);
+ if (err)
+ goto fail;
+
+ tag = fdt_next_tag(fdt,propoffset, NULL);
+ if (tag != FDT_PROP) {
+ err = -FDT_ERR_BADOFFSET;
+ goto fail;
+ }
+
+ err = -FDT_ERR_BADSTRUCTURE;
+ prop = fdt_offset_ptr(fdt, propoffset, sizeof(*prop));
+ if (!prop)
+ goto fail;
+
+ if (name) {
+ namestroff = fdt32_to_cpu(prop->nameoff);
+ *name = fdt_string(fdt, namestroff);
+ }
+
+ len = fdt32_to_cpu(prop->len);
+ if (lenp)
+ *lenp = len;
+
+ prop = fdt_offset_ptr(fdt, propoffset, sizeof(*prop) + len);
+ if (!prop)
+ goto fail;
+
+ return prop->data;
+
+fail:
+ if (lenp)
+ *lenp = err;
+
+ return NULL;
+}
+
uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
{
const uint32_t *php;
@@ -581,3 +626,92 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
return -FDT_ERR_NOTFOUND;
}
+
+int fdt_get_next_node(const void *fdt, int startoffset,
+ int *depth, int recursive)
+{
+ uint32_t tag;
+ int offset, nextoffset;
+
+ CHECK_HEADER(fdt);
+
+ if (startoffset >= 0) {
+ tag = fdt_next_tag(fdt, startoffset, &nextoffset);
+ if (tag != FDT_BEGIN_NODE)
+ return -FDT_ERR_BADOFFSET;
+ } else {
+ nextoffset = 0;
+ }
+
+ do {
+ offset = nextoffset;
+ tag = fdt_next_tag(fdt, offset, &nextoffset);
+
+ switch (tag) {
+ case FDT_BEGIN_NODE:
+ if ((*depth)++ == 0 || recursive) {
+ return offset;
+ }
+
+ break;
+
+ case FDT_END_NODE:
+ if (--*depth < 0)
+ return -FDT_ERR_NOTFOUND;
+
+ break;
+
+ case FDT_PROP:
+ case FDT_END:
+ case FDT_NOP:
+ break;
+
+ default:
+ return -FDT_ERR_BADSTRUCTURE;
+ }
+ } while (tag != FDT_END);
+
+ if (depth != 0)
+ return -FDT_ERR_BADSTRUCTURE;
+
+ return -FDT_ERR_NOTFOUND;
+}
+
+int fdt_get_next_prop(const void *fdt, int startoffset)
+{
+ uint32_t tag;
+ int offset, nextoffset;
+
+ CHECK_HEADER(fdt);
+
+ if (startoffset >= 0) {
+ tag = fdt_next_tag(fdt, startoffset, &nextoffset);
+ if (tag != FDT_BEGIN_NODE && tag != FDT_PROP)
+ return -FDT_ERR_BADOFFSET;
+ } else {
+ nextoffset = 0;
+ }
+
+ do {
+ offset = nextoffset;
+ tag = fdt_next_tag(fdt, offset, &nextoffset);
+
+ switch (tag) {
+ case FDT_BEGIN_NODE:
+ case FDT_END_NODE:
+ return -FDT_ERR_NOTFOUND;
+
+ case FDT_PROP:
+ return offset;
+
+ case FDT_END:
+ case FDT_NOP:
+ break;
+
+ default:
+ return -FDT_ERR_BADSTRUCTURE;
+ }
+ } while (tag != FDT_END);
+
+ return -FDT_ERR_BADSTRUCTURE;
+}
diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
index f9d32ef..4e87550 100644
--- a/libfdt/fdt_strerror.c
+++ b/libfdt/fdt_strerror.c
@@ -70,6 +70,7 @@ static struct errtabent errtable[] = {
ERRTABENT(FDT_ERR_BADOFFSET),
ERRTABENT(FDT_ERR_BADPATH),
ERRTABENT(FDT_ERR_BADSTATE),
+ ERRTABENT(FDT_ERR_BADDEPTH),
ERRTABENT(FDT_ERR_TRUNCATED),
ERRTABENT(FDT_ERR_BADMAGIC),
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index d053689..6c5d4a9 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -85,25 +85,28 @@
/* FDT_ERR_BADSTATE: Function was passed an incomplete device
* tree created by the sequential-write functions, which is
* not sufficiently complete for the requested operation. */
+#define FDT_ERR_BADDEPTH 8
+ /* FDT_ERR_BADDEPTH: Function was passed a negative
+ * (or otherwise invalid) depth. */
/* Error codes: codes for bad device tree blobs */
-#define FDT_ERR_TRUNCATED 8
+#define FDT_ERR_TRUNCATED 9
/* FDT_ERR_TRUNCATED: Structure block of the given device tree
* ends without an FDT_END tag. */
-#define FDT_ERR_BADMAGIC 9
+#define FDT_ERR_BADMAGIC 10
/* FDT_ERR_BADMAGIC: Given "device tree" appears not to be a
* device tree at all - it is missing the flattened device
* tree magic number. */
-#define FDT_ERR_BADVERSION 10
+#define FDT_ERR_BADVERSION 11
/* FDT_ERR_BADVERSION: Given device tree has a version which
* can't be handled by the requested operation. For
* read-write functions, this may mean that fdt_open_into() is
* required to convert the tree to the expected version. */
-#define FDT_ERR_BADSTRUCTURE 11
+#define FDT_ERR_BADSTRUCTURE 12
/* FDT_ERR_BADSTRUCTURE: Given device tree has a corrupt
* structure block or other serious error (e.g. misnested
* nodes, or subnodes preceding properties). */
-#define FDT_ERR_BADLAYOUT 12
+#define FDT_ERR_BADLAYOUT 13
/* FDT_ERR_BADLAYOUT: For read-write functions, the given
* device tree has it's sub-blocks in an order that the
* function can't handle (memory reserve map, then structure,
@@ -111,12 +114,12 @@
* into a form suitable for the read-write operations. */
/* "Can't happen" error indicating a bug in libfdt */
-#define FDT_ERR_INTERNAL 13
+#define FDT_ERR_INTERNAL 14
/* FDT_ERR_INTERNAL: libfdt has failed an internal assertion.
* Should never be returned, if it is, it indicates a bug in
* libfdt itself. */
-#define FDT_ERR_MAX 13
+#define FDT_ERR_MAX 14
/**********************************************************************/
/* Low-level functions (you probably don't need these) */
@@ -409,6 +412,41 @@ static inline void *fdt_getprop_w(void *fdt, int nodeoffset,
}
/**
+ * fdt_getprop_offset - retrieve the value of a given property by offset
+ * @fdt: pointer to the device tree blob
+ * @propoffset: offset of the property to read
+ * @name: pointer to a character pointer (will be overwritten) or NULL
+ * @lenp: pointer to an integer variable (will be overwritten) or NULL
+ *
+ * fdt_getprop() retrieves a pointer to the value of the property
+ * named 'name' of the node at offset nodeoffset (this will be a
+ * pointer to within the device blob itself, not a copy of the value).
+ * If lenp is non-NULL, the length of the property value also
+ * returned, in the integer pointed to by lenp.
+ *
+ * returns:
+ * pointer to the property's value
+ * if lenp is non-NULL, *lenp contains the length of the property
+ * value (>=0)
+ * NULL, on error
+ * if lenp is non-NULL, *lenp contains an error code (<0):
+ * -FDT_ERR_NOTFOUND, node does not have named property
+ * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ * -FDT_ERR_BADMAGIC,
+ * -FDT_ERR_BADVERSION,
+ * -FDT_ERR_BADSTATE,
+ * -FDT_ERR_BADSTRUCTURE,
+ * -FDT_ERR_TRUNCATED, standard meanings
+ */
+const void *fdt_getprop_offset(const void *fdt, int propoffset,
+ const char **name, int *lenp);
+static inline void *fdt_getprop_offset_w(void *fdt, int propoffset,
+ const char **name, int *lenp)
+{
+ return (void *)fdt_getprop_offset(fdt, propoffset, name, lenp);
+}
+
+/**
* fdt_get_phandle - retreive the phandle of a given node
* @fdt: pointer to the device tree blob
* @nodeoffset: structure block offset of the node
@@ -651,6 +689,77 @@ int fdt_node_check_compatible(const void *fdt, int nodeoffset,
int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
const char *compatible);
+/**
+ * fdt_get_next_node - enumerate children of a node
+ * @fdt: pointer to the device tree blob
+ * @startoffset: only find nodes after this offset
+ * @depth: depth of the node, should be zero upon first call
+ * @recursive: only find immediate children if zero
+ *
+ * fdt_get_next_node() returns the offset of the first node after
+ * startoffset, until depth returns to zero.
+ *
+ * To iterate through all children, the following idiom can be used:
+ * depth = 0;
+ * offset = parent node offset;
+ * while (1) {
+ * offset = fdt_get_next_node(fdt, offset, &depth, recursive);
+ * if (offset < 0)
+ * break;
+ *
+ * // other code here
+ * }
+ *
+ * To find all the children of the root node, set the initial
+ * offset to zero. To find all nodes *including* the root
+ * node, set the initial offset and depth to -1.
+ *
+ * returns:
+ * structure block offset of the located node (>= 0, >startoffset),
+ * on success
+ * -FDT_ERR_NOTFOUND, no more child nodes exist after startoffset
+ * -FDT_ERR_BADOFFSET, nodeoffset does not refer to a BEGIN_NODE tag
+ * -FDT_ERR_BADMAGIC,
+ * -FDT_ERR_BADVERSION,
+ * -FDT_ERR_BADSTATE,
+ * -FDT_ERR_BADSTRUCTURE,
+ * -FDT_ERR_BADDEPTH, standard meanings
+ */
+int fdt_get_next_node(const void *fdt, int startoffset,
+ int *depth, int recursive);
+
+/**
+ * fdt_get_next_prop - enumerate properties of a node
+ * @fdt: pointer to the device tree blob
+ * @startoffset: only find nodes after this offset
+ *
+ * fdt_get_next_prop() returns the offset of the first property after
+ * startoffset.
+ *
+ * To iterate through all properties, the following idiom can be used:
+ * offset = node offset;
+ * while (1) {
+ * offset = fdt_get_next_prop(fdt, offset);
+ * if (offset < 0)
+ * break;
+ *
+ * // other code here
+ * }
+ *
+ * returns:
+ * structure block offset of the located property (>= 0, >startoffset),
+ * on success
+ * -FDT_ERR_NOTFOUND, no more properties exist in the current node
+ * after startoffset
+ * -FDT_ERR_BADOFFSET, nodeoffset does not refer to a BEGIN_NODE tag
+ * or a PROP tag
+ * -FDT_ERR_BADMAGIC,
+ * -FDT_ERR_BADVERSION,
+ * -FDT_ERR_BADSTATE,
+ * -FDT_ERR_BADSTRUCTURE, standard meanings
+ */
+int fdt_get_next_prop(const void *fdt, int startoffset);
+
/**********************************************************************/
/* Write-in-place functions */
/**********************************************************************/
--
1.5.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-14 16:30 [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset() Scott Wood
@ 2008-01-14 22:31 ` Stephen Rothwell
2008-01-14 22:44 ` Scott Wood
2008-01-15 0:16 ` David Gibson
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Rothwell @ 2008-01-14 22:31 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, jdl
[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]
Hi Scott,
On Mon, 14 Jan 2008 10:30:04 -0600 Scott Wood <scottwood@freescale.com> wrote:
>
> ft_get_next_node() enumerates children of a given node.
> ft_get_next_prop() enumerates propreties of a given node.
^^^^^^^^^^
typo.
> ft_getprop_offset() is like ft_getprop(), but takes a property offset rather
> than a node offset and property name; it is primarily intended for use
> with ft_get_next_prop().
All the "ft_" above should be "fdt_".
> +const void *fdt_getprop_offset(const void *fdt, int propoffset,
> + const char **name, int *lenp)
> +{
> + tag = fdt_next_tag(fdt,propoffset, NULL);
^
space after ','
> +++ b/libfdt/libfdt.h
> +#define FDT_ERR_BADDEPTH 8
Wouldn't it have been less intrusive to just use the next error number
rather than inserting this here?
> + * fdt_getprop_offset - retrieve the value of a given property by offset
> + * @fdt: pointer to the device tree blob
> + * @propoffset: offset of the property to read
> + * @name: pointer to a character pointer (will be overwritten) or NULL
^^^^^^^^^^^^^^^^^
"string"?
> + * @lenp: pointer to an integer variable (will be overwritten) or NULL
> + *
> + * fdt_getprop() retrieves a pointer to the value of the property
fdt_getprop_offset
> + * named 'name' of the node at offset nodeoffset (this will be a
> + * pointer to within the device blob itself, not a copy of the value).
> + * If lenp is non-NULL, the length of the property value also
> + * returned, in the integer pointed to by lenp.
> + *
> + * returns:
> + * pointer to the property's value
if name is non-NULL, *name points to ...
> + * if lenp is non-NULL, *lenp contains the length of the property
> + * value (>=0)
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-14 22:31 ` Stephen Rothwell
@ 2008-01-14 22:44 ` Scott Wood
2008-01-14 22:54 ` Stephen Rothwell
0 siblings, 1 reply; 8+ messages in thread
From: Scott Wood @ 2008-01-14 22:44 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linuxppc-dev, jdl
Stephen Rothwell wrote:
>> +++ b/libfdt/libfdt.h
>
>> +#define FDT_ERR_BADDEPTH 8
>
> Wouldn't it have been less intrusive to just use the next error number
> rather than inserting this here?
Yes, but then either the order in errtable[] wouldn't match the order in
the header file, or the error type grouping would be broken.
If we want to maintain such a grouping, we should probably leave some
number space between the groups.
>> + * fdt_getprop_offset - retrieve the value of a given property by offset
>> + * @fdt: pointer to the device tree blob
>> + * @propoffset: offset of the property to read
>> + * @name: pointer to a character pointer (will be overwritten) or NULL
> ^^^^^^^^^^^^^^^^^
> "string"?
"pointer to a string" could be interpreted as "char *", not "char **".
I'll fix the others; thanks for pointing them out.
-Scott
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-14 22:44 ` Scott Wood
@ 2008-01-14 22:54 ` Stephen Rothwell
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Rothwell @ 2008-01-14 22:54 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, jdl
[-- Attachment #1: Type: text/plain, Size: 863 bytes --]
On Mon, 14 Jan 2008 16:44:33 -0600 Scott Wood <scottwood@freescale.com> wrote:
>
> Stephen Rothwell wrote:
> >> +++ b/libfdt/libfdt.h
> >
> >> +#define FDT_ERR_BADDEPTH 8
> >
> > Wouldn't it have been less intrusive to just use the next error number
> > rather than inserting this here?
>
> Yes, but then either the order in errtable[] wouldn't match the order in
> the header file, or the error type grouping would be broken.
>
> If we want to maintain such a grouping, we should probably leave some
> number space between the groups.
OK.
> "pointer to a string" could be interpreted as "char *", not "char **".
Yeah, it has always been a bit ambiguous.
> I'll fix the others; thanks for pointing them out.
No worries.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-14 16:30 [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset() Scott Wood
2008-01-14 22:31 ` Stephen Rothwell
@ 2008-01-15 0:16 ` David Gibson
2008-01-15 16:52 ` Scott Wood
1 sibling, 1 reply; 8+ messages in thread
From: David Gibson @ 2008-01-15 0:16 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, jdl
On Mon, Jan 14, 2008 at 10:30:04AM -0600, Scott Wood wrote:
> ft_get_next_node() enumerates children of a given node.
> ft_get_next_prop() enumerates propreties of a given node.
>
> ft_getprop_offset() is like ft_getprop(), but takes a property offset rather
> than a node offset and property name; it is primarily intended for use
> with ft_get_next_prop().
Urg... this kind of serves me right for not getting my act together on
iterator functions yet. I really don't like this approach much. I'll
see if I can come up with something I prefer this afternoon.
The biggest thing I dislike is that I've deliberately avoided having
any offsets-to-properties exposed to the library user. That's because
the whole offsets change when you write the tree problem is worse for
properties than nodes (in that likely idiomatic uses will be bitten by
changes in property offsets).
Some more specific comments on the patch below.
[snip]
> +const void *fdt_getprop_offset(const void *fdt, int propoffset,
> + const char **name, int *lenp)
> +{
> + uint32_t tag;
> + const struct fdt_property *prop;
> + int namestroff;
> + int err, len;
> +
> + err = fdt_check_header(fdt);
> + if (err)
> + goto fail;
> +
> + tag = fdt_next_tag(fdt,propoffset, NULL);
> + if (tag != FDT_PROP) {
> + err = -FDT_ERR_BADOFFSET;
> + goto fail;
> + }
> +
> + err = -FDT_ERR_BADSTRUCTURE;
> + prop = fdt_offset_ptr(fdt, propoffset, sizeof(*prop));
> + if (!prop)
> + goto fail;
> +
> + if (name) {
> + namestroff = fdt32_to_cpu(prop->nameoff);
> + *name = fdt_string(fdt, namestroff);
> + }
> +
> + len = fdt32_to_cpu(prop->len);
> + if (lenp)
> + *lenp = len;
> +
> + prop = fdt_offset_ptr(fdt, propoffset, sizeof(*prop) + len);
> + if (!prop)
> + goto fail;
This juggling to call fdt_offset_ptr() over the whole length of the
property representation should be rolled up into an internal function
and used within fdt_get_property() as well.
[snip]
> +int fdt_get_next_node(const void *fdt, int startoffset,
> + int *depth, int recursive)
> +{
> + uint32_t tag;
> + int offset, nextoffset;
> +
> + CHECK_HEADER(fdt);
> +
> + if (startoffset >= 0) {
> + tag = fdt_next_tag(fdt, startoffset, &nextoffset);
> + if (tag != FDT_BEGIN_NODE)
> + return -FDT_ERR_BADOFFSET;
> + } else {
> + nextoffset = 0;
> + }
> +
> + do {
> + offset = nextoffset;
> + tag = fdt_next_tag(fdt, offset, &nextoffset);
> +
> + switch (tag) {
> + case FDT_BEGIN_NODE:
> + if ((*depth)++ == 0 || recursive) {
> + return offset;
> + }
> +
> + break;
> +
> + case FDT_END_NODE:
> + if (--*depth < 0)
> + return -FDT_ERR_NOTFOUND;
> +
> + break;
> +
> + case FDT_PROP:
> + case FDT_END:
> + case FDT_NOP:
> + break;
> +
> + default:
> + return -FDT_ERR_BADSTRUCTURE;
> + }
> + } while (tag != FDT_END);
> +
> + if (depth != 0)
> + return -FDT_ERR_BADSTRUCTURE;
I think this should be FDT_ERR_TRUNCATED.
> + return -FDT_ERR_NOTFOUND;
In fact, so should this. This function should never actually reach
the FDT_END tag.
> +}
> +
> +int fdt_get_next_prop(const void *fdt, int startoffset)
> +{
> + uint32_t tag;
> + int offset, nextoffset;
> +
> + CHECK_HEADER(fdt);
> +
> + if (startoffset >= 0) {
> + tag = fdt_next_tag(fdt, startoffset, &nextoffset);
> + if (tag != FDT_BEGIN_NODE && tag != FDT_PROP)
> + return -FDT_ERR_BADOFFSET;
> + } else {
> + nextoffset = 0;
This alternate case shouldn't be here. For properties, the given
offset should always be either the node offset to find within, or
another property offset. This other case simply makes negative and 0
startoffset equivalent. Instead, negative startoffset should cause
FDT_ERR_BADOFFSET.
> + }
> +
> + do {
> + offset = nextoffset;
> + tag = fdt_next_tag(fdt, offset, &nextoffset);
> +
> + switch (tag) {
> + case FDT_BEGIN_NODE:
> + case FDT_END_NODE:
> + return -FDT_ERR_NOTFOUND;
> +
> + case FDT_PROP:
> + return offset;
> +
> + case FDT_END:
> + case FDT_NOP:
> + break;
> +
> + default:
> + return -FDT_ERR_BADSTRUCTURE;
> + }
> + } while (tag != FDT_END);
> +
> + return -FDT_ERR_BADSTRUCTURE;
Should be FDT_ERR_TRUNCATED again.
> +}
> diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
> index f9d32ef..4e87550 100644
> --- a/libfdt/fdt_strerror.c
> +++ b/libfdt/fdt_strerror.c
> @@ -70,6 +70,7 @@ static struct errtabent errtable[] = {
> ERRTABENT(FDT_ERR_BADOFFSET),
> ERRTABENT(FDT_ERR_BADPATH),
> ERRTABENT(FDT_ERR_BADSTATE),
> + ERRTABENT(FDT_ERR_BADDEPTH),
>
> ERRTABENT(FDT_ERR_TRUNCATED),
> ERRTABENT(FDT_ERR_BADMAGIC),
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index d053689..6c5d4a9 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -85,25 +85,28 @@
> /* FDT_ERR_BADSTATE: Function was passed an incomplete device
> * tree created by the sequential-write functions, which is
> * not sufficiently complete for the requested operation. */
> +#define FDT_ERR_BADDEPTH 8
> + /* FDT_ERR_BADDEPTH: Function was passed a negative
> + * (or otherwise invalid) depth. */
You've added this error code, but you don't actually return it
anywhere...
[snip]
> /**
> + * fdt_getprop_offset - retrieve the value of a given property by offset
> + * @fdt: pointer to the device tree blob
> + * @propoffset: offset of the property to read
> + * @name: pointer to a character pointer (will be overwritten) or NULL
> + * @lenp: pointer to an integer variable (will be overwritten) or NULL
> + *
> + * fdt_getprop() retrieves a pointer to the value of the property
> + * named 'name' of the node at offset nodeoffset (this will be a
> + * pointer to within the device blob itself, not a copy of the value).
> + * If lenp is non-NULL, the length of the property value also
> + * returned, in the integer pointed to by lenp.
This description is incorrect - you've copied the fdt_getprop()
description and forgotten to update it.
[snip]
And finally, new libfdt functions should have testcases.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-15 0:16 ` David Gibson
@ 2008-01-15 16:52 ` Scott Wood
2008-01-16 3:49 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Scott Wood @ 2008-01-15 16:52 UTC (permalink / raw)
To: jdl, linuxppc-dev
On Tue, Jan 15, 2008 at 11:16:57AM +1100, David Gibson wrote:
> On Mon, Jan 14, 2008 at 10:30:04AM -0600, Scott Wood wrote:
> > ft_get_next_node() enumerates children of a given node.
> > ft_get_next_prop() enumerates propreties of a given node.
> >
> > ft_getprop_offset() is like ft_getprop(), but takes a property offset rather
> > than a node offset and property name; it is primarily intended for use
> > with ft_get_next_prop().
>
> Urg... this kind of serves me right for not getting my act together on
> iterator functions yet. I really don't like this approach much. I'll
> see if I can come up with something I prefer this afternoon.
OK.
> The biggest thing I dislike is that I've deliberately avoided having
> any offsets-to-properties exposed to the library user. That's because
> the whole offsets change when you write the tree problem is worse for
> properties than nodes (in that likely idiomatic uses will be bitten by
> changes in property offsets).
We can drop the property iteration for now; I wrote it for an attempt at
node merging which I postponed.
> > + if (depth != 0)
> > + return -FDT_ERR_BADSTRUCTURE;
>
> I think this should be FDT_ERR_TRUNCATED.
I'd expect TRUNCATED when we hit totalsize without an FDT_END, not when
we hit FDT_END in an inappropriate context.
> > + return -FDT_ERR_NOTFOUND;
>
> In fact, so should this. This function should never actually reach
> the FDT_END tag.
No, it's valid if you're iterating over all nodes (node -1, depth -1).
> > +#define FDT_ERR_BADDEPTH 8
> > + /* FDT_ERR_BADDEPTH: Function was passed a negative
> > + * (or otherwise invalid) depth. */
>
> You've added this error code, but you don't actually return it
> anywhere...
Oops, it must have been from the aforementioned node merging code, and I
thought it was from fdt_get_next_node when splitting things up.
> This description is incorrect - you've copied the fdt_getprop()
> description and forgotten to update it.
/me hides in shame
-Scott
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-15 16:52 ` Scott Wood
@ 2008-01-16 3:49 ` David Gibson
2008-01-16 7:15 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2008-01-16 3:49 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, jdl
On Tue, Jan 15, 2008 at 10:52:30AM -0600, Scott Wood wrote:
> On Tue, Jan 15, 2008 at 11:16:57AM +1100, David Gibson wrote:
> > On Mon, Jan 14, 2008 at 10:30:04AM -0600, Scott Wood wrote:
> > > ft_get_next_node() enumerates children of a given node.
> > > ft_get_next_prop() enumerates propreties of a given node.
> > >
> > > ft_getprop_offset() is like ft_getprop(), but takes a property offset rather
> > > than a node offset and property name; it is primarily intended for use
> > > with ft_get_next_prop().
> >
> > Urg... this kind of serves me right for not getting my act together on
> > iterator functions yet. I really don't like this approach much. I'll
> > see if I can come up with something I prefer this afternoon.
>
> OK.
>
> > The biggest thing I dislike is that I've deliberately avoided having
> > any offsets-to-properties exposed to the library user. That's because
> > the whole offsets change when you write the tree problem is worse for
> > properties than nodes (in that likely idiomatic uses will be bitten by
> > changes in property offsets).
>
> We can drop the property iteration for now; I wrote it for an attempt at
> node merging which I postponed.
Heh.. and I spent most of yesterday working on a counter-proposal for
property iteration. Oh well, let's leave this for now and look at the
node iteration first.
> > > + if (depth != 0)
> > > + return -FDT_ERR_BADSTRUCTURE;
> >
> > I think this should be FDT_ERR_TRUNCATED.
>
> I'd expect TRUNCATED when we hit totalsize without an FDT_END, not when
> we hit FDT_END in an inappropriate context.
Well, TRUNCATED is valid for either situation, and fdt_next_tag() also
returns FDT_END if it reaches totalsize.
> > > + return -FDT_ERR_NOTFOUND;
> >
> > In fact, so should this. This function should never actually reach
> > the FDT_END tag.
>
> No, it's valid if you're iterating over all nodes (node -1, depth
> -1).
Uh.. yeah, I guess so.
> > > +#define FDT_ERR_BADDEPTH 8
> > > + /* FDT_ERR_BADDEPTH: Function was passed a negative
> > > + * (or otherwise invalid) depth. */
> >
> > You've added this error code, but you don't actually return it
> > anywhere...
>
> Oops, it must have been from the aforementioned node merging code, and I
> thought it was from fdt_get_next_node when splitting things up.
Ok.
> > This description is incorrect - you've copied the fdt_getprop()
> > description and forgotten to update it.
>
> /me hides in shame
:)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset().
2008-01-16 3:49 ` David Gibson
@ 2008-01-16 7:15 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2008-01-16 7:15 UTC (permalink / raw)
To: Scott Wood, jdl, linuxppc-dev
On Wed, Jan 16, 2008 at 02:49:39PM +1100, David Gibson wrote:
> On Tue, Jan 15, 2008 at 10:52:30AM -0600, Scott Wood wrote:
> > On Tue, Jan 15, 2008 at 11:16:57AM +1100, David Gibson wrote:
> > > On Mon, Jan 14, 2008 at 10:30:04AM -0600, Scott Wood wrote:
[snip]
> > > > + if (depth != 0)
> > > > + return -FDT_ERR_BADSTRUCTURE;
> > >
> > > I think this should be FDT_ERR_TRUNCATED.
> >
> > I'd expect TRUNCATED when we hit totalsize without an FDT_END, not when
> > we hit FDT_END in an inappropriate context.
>
> Well, TRUNCATED is valid for either situation, and fdt_next_tag() also
> returns FDT_END if it reaches totalsize.
Thought I should elaborate on this a bit - I'm not sure that the
current handling of FDT_ERR_TRUNCATED is entirely correct. The
description says only when the FDT_END tag is missing, but the way
fdt_next_tag() works means we will also (for some functions, at least)
give this error when FDT_END appears without a suitable number of
FDT_END_NODE tags preceding it.
The rationale behind FDT_ERR_TRUNCATED - why it exists as a separate
code from FDT_ERR_BADSTRUCTURE - is that the read-only functions
should work more-or-less sanely on incomplete trees created by the
sequential-write functions (fdt_sw.c). FDT_ERR_TRUNCATED is supposed
to indicate that the function couldn't complete properly because the
tree is malformed, but that suitable calls to the fdt_sw functions
could complete the tree so that it's no longer malformed.
I'm not sure that either the comment describing TRUNCATED, or the code
entirely matches that rationale...
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-01-16 7:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-14 16:30 [DTC PATCH] libfdt: Add ft_get_next_node(), ft_get_next_prop(), and ft_getprop_offset() Scott Wood
2008-01-14 22:31 ` Stephen Rothwell
2008-01-14 22:44 ` Scott Wood
2008-01-14 22:54 ` Stephen Rothwell
2008-01-15 0:16 ` David Gibson
2008-01-15 16:52 ` Scott Wood
2008-01-16 3:49 ` David Gibson
2008-01-16 7:15 ` David Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).