common flatdevtree code

Hi Hollis,

I'm doing some fairly massive rework to my patches so it'll take
another day or two (plus 4 day weekend for me).  In the meantime,
this is what I've done to your code.  :)

I still plan on changing it a bit to use ft_next in a few more routines
(e.g., ft_dump_blob).  ft_next has a clumsy interface but I like the fact
that it separates the "how to traverse the nodes/properties in the tree"
knowledge from the "what do I want to do with this particular node/property"
knowledge.  Also, if/when version 0x11 (or whatever) comes along, we only
have to change one routine to be able to correctly traverse the tree.

I've included flatdevtree.[ch] and the flatdevtree_env.h for the
bootwrapper for reference.  I didn't make one for you but I can.

Its a work in progress but let me know if you have any issues so far.

Thanks,

Mark

Mark A. Greer writes:

I'm doing some fairly massive rework to my patches so it'll take
another day or two (plus 4 day weekend for me).  In the meantime,
this is what I've done to your code.  :)

Any further progress on this?  I need an ft library for the prep
bootwrapper changes I'm planning.

I don't like the approach of doing a malloc for the whole blob every
time you make a property larger though.  I'm going to be adding quite
a few nodes and properties, potentially, and with a simple-minded
malloc (such as you get in a bootwrapper) it's likely to chew through
a lot of memory unnecessarily.  I would rather be able to start with
an existing blob and pull it apart once, add stuff to it, then put it
back together, doing a small number of mallocs in the process.

I still plan on changing it a bit to use ft_next in a few more routines
(e.g., ft_dump_blob).  ft_next has a clumsy interface but I like the fact

The interface might look nicer if we had a structure for the tag,
name, value and length.  Then there would only be one parameter
instead of the four you have at the moment.

Paul.

On Wed, Sep 06, 2006 at 01:52:07PM +1000, Paul Mackerras wrote:

Hi Paul,

Mark A. Greer writes:

quoted

I'm doing some fairly massive rework to my patches so it'll take
another day or two (plus 4 day weekend for me).  In the meantime,
this is what I've done to your code.  :)

Any further progress on this?  I need an ft library for the prep
bootwrapper changes I'm planning.

Yes, I'm preparing patches right now (need a few more hours though).

I don't like the approach of doing a malloc for the whole blob every
time you make a property larger though.  I'm going to be adding quite
a few nodes and properties, potentially, and with a simple-minded
malloc (such as you get in a bootwrapper) it's likely to chew through
a lot of memory unnecessarily.  I would rather be able to start with
an existing blob and pull it apart once, add stuff to it, then put it
back together, doing a small number of mallocs in the process.

Yes, we definitely need a smarter approach.  I'll post the patches of
what I have now and we can go from there.

quoted

I still plan on changing it a bit to use ft_next in a few more routines
(e.g., ft_dump_blob).  ft_next has a clumsy interface but I like the fact

The interface might look nicer if we had a structure for the tag,
name, value and length.  Then there would only be one parameter
instead of the four you have at the moment.

That's a good idea.  I'll do that before posting the patches.

Mark

On Wed, Sep 06, 2006 at 01:52:07PM +1000, Paul Mackerras wrote:

Mark A. Greer writes:

quoted

I'm doing some fairly massive rework to my patches so it'll take
another day or two (plus 4 day weekend for me).  In the meantime,
this is what I've done to your code.  :)

Any further progress on this?  I need an ft library for the prep
bootwrapper changes I'm planning.

I don't like the approach of doing a malloc for the whole blob every
time you make a property larger though.  I'm going to be adding quite
a few nodes and properties, potentially, and with a simple-minded
malloc (such as you get in a bootwrapper) it's likely to chew through
a lot of memory unnecessarily.  I would rather be able to start with
an existing blob and pull it apart once, add stuff to it, then put it
back together, doing a small number of mallocs in the process.

quoted

I still plan on changing it a bit to use ft_next in a few more routines
(e.g., ft_dump_blob).  ft_next has a clumsy interface but I like the fact

The interface might look nicer if we had a structure for the tag,
name, value and length.  Then there would only be one parameter
instead of the four you have at the moment.

Paul.

Paul,

Here's what I have at the moment...

- flatdevtree.[ch] are copies of what Hollis will be keeping (but I've
edited the ones attached to this email)

- flatdevtree_env.h is a bootwrapper hdr file that contains local
  defines that the generic flatdevtree.[ch] need

- flatdevtree_misc.c is other ft stuff that isn't in flatdevtree.c plus
  some bootwrapper->flatdevtree.c glue code

Mark
--

Thanks Mark!

On Wed, 2006-09-06 at 17:36 -0700, Mark A. Greer wrote:

void *ft_find_device(const void *bphp, const char *srch_path)

How about "ft_find_node"? I renamed that and made a couple other small
changes. In particular:
- u8->int
- exit and free -> ft_exit and ft_free (so they can be wrapped in
flatdevtree_env.h)
- moved those inlines out of flatdevtree.h, since they don't need to be
exported

I just put up a Mercurial tree. I didn't include your flatdevtree_misc.c
since it's kernel-specific, and I added a userspace flatdevtree_env.h.

To get the source:
hg clone http://unsanctioned.org/flatdevtree/hgweb.py flatdevtree

I'd like to add some unit tests in the near future (e.g. comparing with
dtc output), but I wasted a couple hours screwing with a makefile
instead. :(

By the way, could you send a patch adding your copyright info?

-Hollis

Hi Hollis.

On Thu, Sep 07, 2006 at 02:43:31AM -0500, Hollis Blanchard wrote:

Thanks Mark!

On Wed, 2006-09-06 at 17:36 -0700, Mark A. Greer wrote:

quoted

void *ft_find_device(const void *bphp, const char *srch_path)

How about "ft_find_node"?

Sure.

I renamed that and made a couple other small
changes. In particular:
- u8->int

OK.

- exit and free -> ft_exit and ft_free (so they can be wrapped in
flatdevtree_env.h)

Makes sense.

- moved those inlines out of flatdevtree.h, since they don't need to be
exported

ft_parentize is also used in flatdevtree_misc.c so I think they
should stay in flatdevtree.h.

I just put up a Mercurial tree. I didn't include your flatdevtree_misc.c
since it's kernel-specific, and I added a userspace flatdevtree_env.h.

To get the source:
hg clone http://unsanctioned.org/flatdevtree/hgweb.py flatdevtree

Oh, cool.

I'd like to add some unit tests in the near future (e.g. comparing with
dtc output), but I wasted a couple hours screwing with a makefile
instead. :(

By the way, could you send a patch adding your copyright info?

Yeah, I'm still cleaning up & testing a bit.  I should have them out
today unless something comes up (well in addition to a scheduled hour
long network outtage :( .

Mark

On Thu, 2006-09-07 at 11:27 -0700, Mark A. Greer wrote:

On Thu, Sep 07, 2006 at 02:43:31AM -0500, Hollis Blanchard wrote:

quoted

On Wed, 2006-09-06 at 17:36 -0700, Mark A. Greer wrote:

quoted

void *ft_find_device(const void *bphp, const char *srch_path)

How about "ft_find_node"?

Sure.

quoted

I renamed that and made a couple other small
changes. In particular:
- u8->int

OK.

quoted

- exit and free -> ft_exit and ft_free (so they can be wrapped in
flatdevtree_env.h)

Makes sense.

quoted

- moved those inlines out of flatdevtree.h, since they don't need to be
exported

ft_parentize is also used in flatdevtree_misc.c so I think they
should stay in flatdevtree.h. 

OK.

I've pushed these changes and a couple more (run hg pull -u to update).

-- 
Hollis Blanchard
IBM Linux Technology Center

Hollis, Paul,

I made some more changes that I hope you approve of:
- Added a hdr to the file to try to make it clear that people should
  not hack the local copy of the file.
- Fixed a booboo (typo) in my last patch
- Change the interface to ft_next() to pass a struct that contains the
  pointers that it sets up.  Also moved the p_strings & version code
  from the caller to ft_next().  Now, just pass in a NULL value to the
  'p' param to start at the top of the tree.
- Cleaned up the callers to ft_next().  I think its much cleaner now.

Mark
--

--- flatdevtree.c	2006-09-07 15:34:39.000000000 -0700
+++ flatdevtree.c.new	2006-09-07 17:40:24.000000000 -0700

@@ -1,4 +1,11 @@
 /*
+ * DO NOT EDIT THIS FILE!!
+ *
+ * The master copy is kept in a mercurial repository that you can clone:
+ *	"hg clone http://unsanctioned.org/flatdevtree/hgweb.py flatdevtree"
+ * Please send patches to Hollis Blanchard <hollisb@us.ibm.com> and
+ * CC: <Linuxppc-dev@ozlabs.org>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or

@@ -15,7 +22,7 @@
  *
  * Copyright Pantelis Antoniou 2006
  * Copyright (C) IBM Corporation 2006
- * 2006 (c) MontaVista, Software, Inc.
+ * 2006 (c) MontaVista Software, Inc.
  *
  * Authors: Pantelis Antoniou <pantelis@embeddedalley.com>
  *          Hollis Blanchard <hollisb@us.ibm.com>

@@ -24,29 +31,42 @@
 
 #include "flatdevtree.h"
 
+struct ft_entity {
+	u32	*tagp;		/* Ptr to tag field */
+	char	*name;		/* Name of node or property; else NULL */
+	char	*datap;		/* Ptr to data, if property; else NULL */
+	u32	*sizep;		/* Ptr to propery size; else NULL */
+};
+
 /* Set ptrs to current one's info; return addr of next one */
-static u32 *ft_next(u32 *p, const u32 *p_strings, const u32 version,
-		u32 **tagpp, char **namepp, char **datapp, u32 **sizepp)
+static u32 *ft_next(u32 *p, const void *bphp, struct ft_entity *ftep)
 {
+	const struct boot_param_header *bph = bphp;
+	const u32 *p_strings = (const u32 *)
+		((const char *)bph + be32_to_cpu(bph->off_dt_strings));
+	const u32 version = be32_to_cpu(bph->version);
 	u32 sz;
 
-	*namepp = NULL;
-	*datapp = NULL;
-	*sizepp = NULL;
-	*tagpp = p;
+	if (p == NULL) /* Start at top of tree */
+		p = (u32 *)((char *)bph + be32_to_cpu(bph->off_dt_struct));
+
+	ftep->name = NULL;
+	ftep->datap = NULL;
+	ftep->sizep = NULL;
+	ftep->tagp = p;
 
 	switch (be32_to_cpu(*p++)) { /* Tag */
 	case OF_DT_BEGIN_NODE:
-		*namepp = (char *)p;
+		ftep->name = (char *)p;
 		p = (u32 *)_ALIGN((unsigned long)p + strlen((char *)p) + 1, 4);
 		break;
 	case OF_DT_PROP:
 		sz = be32_to_cpu(*p);
-		*sizepp = p++;
-		*namepp = (char *)p_strings + be32_to_cpu(*p++);
+		ftep->sizep = p++;
+		ftep->name = (char *)p_strings + be32_to_cpu(*p++);
 		if ((version < 0x10) && (sz >= 8))
 			p = (u32 *)_ALIGN((unsigned long)p, 8);
-		*datapp = (char *)p;
+		ftep->datap = (char *)p;
 		p = (u32 *)_ALIGN((unsigned long)p + sz, 4);
 		break;
 	case OF_DT_END_NODE:

@@ -371,13 +391,8 @@ void ft_dump_blob(const void *bphp)
 	const struct boot_param_header *bph = bphp;
 	const u64 *p_rsvmap = (const u64 *)
 		((const char *)bph + be32_to_cpu(bph->off_mem_rsvmap));
-	const u32 *p_struct = (const u32 *)
-		((const char *)bph + be32_to_cpu(bph->off_dt_struct));
-	const u32 *p_strings = (const u32 *)
-		((const char *)bph + be32_to_cpu(bph->off_dt_strings));
-	const u32 version = be32_to_cpu(bph->version);
-	u32 i, *p, *tagp, *sizep;
-	char *namep, *datap;
+	struct ft_entity fte;
+	u32 i, *p;
 	int depth, shift;
 	u64 addr, size;
 

@@ -399,12 +414,11 @@ void ft_dump_blob(const void *bphp)
 		printf("/memreserve/ 0x%llx 0x%llx;\n", addr, size);
 	}
 
-	p = (u32 *)p_struct;
-	while ((p = ft_next(p, p_strings, version, &tagp, &namep, &datap,
-					&sizep)) != NULL)
-		switch (be32_to_cpu(*tagp)) {
+	p = NULL;
+	while ((p = ft_next(p, bphp, &fte)) != NULL)
+		switch (be32_to_cpu(*fte.tagp)) {
 		case OF_DT_BEGIN_NODE:
-			printf("%*s%s {\n", depth * shift, "", namep);
+			printf("%*s%s {\n", depth * shift, "", fte.name);
 			depth++;
 			break;
 		case OF_DT_END_NODE:

@@ -417,13 +431,13 @@ void ft_dump_blob(const void *bphp)
 		case OF_DT_END:
 			break;
 		case OF_DT_PROP:
-			printf("%*s%s", depth * shift, "", namep);
-			print_data(datap, *sizep);
+			printf("%*s%s", depth * shift, "", fte.name);
+			print_data(fte.datap, *fte.sizep);
 			printf(";\n");
 			break;
 		default:
 			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n",
-				depth * shift, "", *tagp);
+				depth * shift, "", *fte.tagp);
 			return;
 		}
 }

@@ -439,13 +453,8 @@ void ft_backtrack_node(struct ft_cxt *cx
 /* note that the root node of the blob is "peeled" off */
 void ft_merge_blob(struct ft_cxt *cxt, void *blob)
 {
-	struct boot_param_header *bph = (struct boot_param_header *)blob;
-	u32 *p_struct = (u32 *) ((char *)bph + be32_to_cpu(bph->off_dt_struct));
-	u32 *p_strings =
-		(u32 *) ((char *)bph + be32_to_cpu(bph->off_dt_strings));
-	const u32 version = be32_to_cpu(bph->version);
-	u32 *p, *tagp, *sizep;
-	char *namep, *datap;
+	struct ft_entity fte;
+	u32 *p;
 	int depth;
 
 	if (be32_to_cpu(*(u32 *) (cxt->p - 4)) != OF_DT_END_NODE)

@@ -454,13 +463,12 @@ void ft_merge_blob(struct ft_cxt *cxt, v
 	cxt->p -= 4;
 
 	depth = 0;
-	p = p_struct;
-	while ((p = ft_next(p, p_strings, version, &tagp, &namep, &datap,
-					&sizep)) != NULL)
-		switch (be32_to_cpu(*tagp)) {
+	p = NULL;
+	while ((p = ft_next(p, blob, &fte)) != NULL)
+		switch (be32_to_cpu(*fte.tagp)) {
 		case OF_DT_BEGIN_NODE:
 			if (depth++ > 0)
-				ft_begin_node(cxt, namep);
+				ft_begin_node(cxt, fte.name);
 			break;
 		case OF_DT_END_NODE:
 			ft_end_node(cxt);

@@ -468,7 +476,7 @@ void ft_merge_blob(struct ft_cxt *cxt, v
 				return;
 			break;
 		case OF_DT_PROP:
-			ft_prop(cxt, namep, datap, *sizep);
+			ft_prop(cxt, fte.name, fte.datap, *fte.sizep);
 			break;
 		}
 }

@@ -477,24 +485,18 @@ void ft_merge_blob(struct ft_cxt *cxt, v
 
 void *ft_find_node(const void *bphp, const char *srch_path)
 {
-	const struct boot_param_header *bph = bphp;
-	u32 *p_struct = (u32 *)((char *)bph + be32_to_cpu(bph->off_dt_struct));
-	u32 *p_strings= (u32 *)((char *)bph + be32_to_cpu(bph->off_dt_strings));
-	u32 version = be32_to_cpu(bph->version);
-	u32 *p, *tagp, *sizep;
-	char *namep, *datap;
+	struct ft_entity fte;
+	u32 *p;
 	static char path[MAX_PATH_LEN];
 
 	path[0] = '\0';
-	p = p_struct;
-
-	while ((p = ft_next(p, p_strings, version, &tagp, &namep, &datap,
-					&sizep)) != NULL)
-		switch (be32_to_cpu(*tagp)) {
+	p = NULL;
+	while ((p = ft_next(p, bphp, &fte)) != NULL)
+		switch (be32_to_cpu(*fte.tagp)) {
 		case OF_DT_BEGIN_NODE:
-			strcat(path, namep);
+			strcat(path, fte.name);
 			if (!strcmp(path, srch_path))
-				return tagp;
+				return fte.tagp;
 			strcat(path, "/");
 			break;
 		case OF_DT_END_NODE:

@@ -507,26 +509,21 @@ void *ft_find_node(const void *bphp, con
 int ft_get_prop(const void *bphp, const void *node, const char *propname,
 		void *buf, const unsigned int buflen)
 {
-	const struct boot_param_header *bph = bphp;
-	u32 *p_strings= (u32 *)((char *)bph + be32_to_cpu(bph->off_dt_strings));
-	u32 version = be32_to_cpu(bph->version);
-	u32 *p, *tagp, *sizep, size;
-	char *namep, *datap;
+	struct ft_entity fte;
+	u32 *p, size;
 	int depth;
 
 	depth = 0;
 	p = (u32 *)node;
-
-	while ((p = ft_next(p, p_strings, version, &tagp, &namep, &datap,
-					&sizep)) != NULL)
-		switch (be32_to_cpu(*tagp)) {
+	while ((p = ft_next(p, bphp, &fte)) != NULL)
+		switch (be32_to_cpu(*fte.tagp)) {
 		case OF_DT_BEGIN_NODE:
 			depth++;
 			break;
 		case OF_DT_PROP:
-			if ((depth == 1) && !strcmp(namep, propname)) {
-				size = min(be32_to_cpu(*sizep), (u32)buflen);
-				memcpy(buf, datap, size);
+			if ((depth == 1) && !strcmp(fte.name, propname)) {
+				size = min(be32_to_cpu(*fte.sizep),(u32)buflen);
+				memcpy(buf, fte.datap, size);
 				return size;
 			}
 			break;

@@ -562,7 +559,7 @@ static void ft_modify_prop(void **bphpp,
 		old_tailp = (u32 *)(datap + old_prop_data_len);
 		new_total_size = head_len + new_prop_data_len + tail_len;
 
-		if (!(new_bph = malloc(new_total_size))) {
+		if (!(new_bph = ft_malloc(new_total_size))) {
 			printf("Can't alloc space for new ft\n");
 			ft_exit(-ENOSPC);
 		}

@@ -604,27 +601,22 @@ static void ft_modify_prop(void **bphpp,
 int ft_set_prop(void **bphpp, const void *node, const char *propname,
 		const void *buf, const unsigned int buflen)
 {
-	struct boot_param_header *bph = *bphpp;
-	u32 *p_strings= (u32 *)((char *)bph + be32_to_cpu(bph->off_dt_strings));
-	u32 version = be32_to_cpu(bph->version);
-	u32 *p, *tagp, *sizep;
-	char *namep, *datap;
+	struct ft_entity fte;
+	u32 *p;
 	int depth;
 
 	depth = 0;
 	p = (u32 *)node;
-
-	while ((p = ft_next(p, p_strings, version, &tagp, &namep, &datap,
-					&sizep)) != NULL)
-		switch (be32_to_cpu(*tagp)) {
+	while ((p = ft_next(p, *bphpp, &fte)) != NULL)
+		switch (be32_to_cpu(*fte.tagp)) {
 		case OF_DT_BEGIN_NODE:
 			depth++;
 			break;
 		case OF_DT_PROP:
-			if ((depth == 1) && !strcmp(namep, propname)) {
-				ft_modify_prop(bphpp, datap, sizep, buf,
+			if ((depth == 1) && !strcmp(fte.name, propname)) {
+				ft_modify_prop(bphpp, fte.datap, fte.sizep, buf,
 						buflen);
-				return be32_to_cpu(*sizep);
+				return be32_to_cpu(*fte.sizep);
 			}
 			break;
 		case OF_DT_END_NODE:

On Thu, Sep 07, 2006 at 05:56:31PM -0700, Mark A. Greer wrote:

Hollis, Paul,

I made some more changes that I hope you approve of:
- Added a hdr to the file to try to make it clear that people should
  not hack the local copy of the file.
- Fixed a booboo (typo) in my last patch
- Change the interface to ft_next() to pass a struct that contains the
  pointers that it sets up.  Also moved the p_strings & version code
  from the caller to ft_next().  Now, just pass in a NULL value to the
  'p' param to start at the top of the tree.
- Cleaned up the callers to ft_next().  I think its much cleaner now.

Forgot to add:

- Change malloc() call to ft_malloc() to be consistent with the changes
  made to exit() -> ft_exit() and free -> ft_free().

Mark

Mark,

Here's where I'm up to...

Paul.

On Fri, Sep 08, 2006 at 11:23:27PM +1000, Paul Mackerras wrote:
Content-Description: message body text

Mark,

Here's where I'm up to...

Wow, you've been busy!

Having a real setprop that can add properties will be nice.  Should slip
into the bootwrapper (w/ my patches applied) pretty easily too.

Thanks for the heads up,

Mark

On Fri, Sep 08, 2006 at 11:23:27PM +1000, Paul Mackerras wrote:
Content-Description: message body text

Mark,

Here's where I'm up to...

Paul.

Paul,

Any update to this or do you want to go with this?  (I haven't tested
it, btw).

Mark

Here's the latest version of my version of the flatdevtree code...

I talked to David Gibson about this and he pointed out that there is a
problem with using a void * to identify nodes (i.e. returned from
ft_find_device and passed to ft_set_prop etc.).  The problem is that
if we reallocate the space for the tree, then any void * handles that
the user of the library has are then invalid.  Perhaps we have to use
offsets from the beginning of the struct region instead - the offset
for a node will be stable across changes of the node or any of its
descendents, at least.

Paul.

On Thu, 2006-09-28 at 08:32 +1000, Paul Mackerras wrote:

Here's the latest version of my version of the flatdevtree code...

I talked to David Gibson about this and he pointed out that there is a
problem with using a void * to identify nodes (i.e. returned from
ft_find_device and passed to ft_set_prop etc.).  The problem is that
if we reallocate the space for the tree, then any void * handles that
the user of the library has are then invalid.  Perhaps we have to use
offsets from the beginning of the struct region instead - the offset
for a node will be stable across changes of the node or any of its
descendents, at least.

That is a non-obvious requirement and therefore a poor API.

Using offsets instead of pointers barely helps. All pointers break if
you need to realloc space for the tree. Almost all offsets break in the
exact same way (except for the ancestors of a particular node).

This may be a good reason to go back to the
	ft_set_prop("/node/property", value, len);
interface, instead of
	n = ft_find_node("/node");
	ft_set_prop(n, "property", value, len);

-- 
Hollis Blanchard
IBM Linux Technology Center

I talked to David Gibson about this and he pointed out that there is a
problem with using a void * to identify nodes (i.e. returned from
ft_find_device and passed to ft_set_prop etc.).  The problem is that
if we reallocate the space for the tree, then any void * handles that
the user of the library has are then invalid.  Perhaps we have to use
offsets from the beginning of the struct region instead - the offset
for a node will be stable across changes of the node or any of its
descendents, at least.

Or unpack the tree before you operate on it.  You can pack it
again later if you need to pass it as a flat tree again (or the
bootwrapper could implement a real OF client interface, which
is useful for many more things!)

A flat contiguous blob of bytes is convenient to pass around,
but not such a great data structure for basically anything else ;-)


Segher

On Thu, 2006-09-28 at 01:41 +0200, Segher Boessenkool wrote:

quoted

I talked to David Gibson about this and he pointed out that there is a
problem with using a void * to identify nodes (i.e. returned from
ft_find_device and passed to ft_set_prop etc.).  The problem is that
if we reallocate the space for the tree, then any void * handles that
the user of the library has are then invalid.  Perhaps we have to use
offsets from the beginning of the struct region instead - the offset
for a node will be stable across changes of the node or any of its
descendents, at least.

Or unpack the tree before you operate on it.  You can pack it
again later if you need to pass it as a flat tree again (or the
bootwrapper could implement a real OF client interface, which
is useful for many more things!)

A flat contiguous blob of bytes is convenient to pass around,
but not such a great data structure for basically anything else ;-)

I agree; I think this data structure's design is explicitly static, and
now we're trying to dynamically edit it.

We essentially need to do unpack/pack now whenever the size of the
structure changes (e.g. changing a property size, adding or removing
anything). Making that explicit (and one-time) might be a good idea:
        tree = ft_unpack(blob);
        n = ft_find_node(tree, "/node");
        ft_set_prop(n, "property", value, len);
        ...
        blob = ft_pack(tree);

-- 
Hollis Blanchard
IBM Linux Technology Center

quoted

Or unpack the tree before you operate on it.  You can pack it
again later if you need to pass it as a flat tree again (or the
bootwrapper could implement a real OF client interface, which
is useful for many more things!)

A flat contiguous blob of bytes is convenient to pass around,
but not such a great data structure for basically anything else ;-)

I agree; I think this data structure's design is explicitly static,  
and
now we're trying to dynamically edit it.

We essentially need to do unpack/pack now whenever the size of the
structure changes (e.g. changing a property size, adding or removing
anything). Making that explicit (and one-time) might be a good idea:
        tree = ft_unpack(blob);
        n = ft_find_node(tree, "/node");
        ft_set_prop(n, "property", value, len);
        ...
        blob = ft_pack(tree);

And then everything but ft_{un,}pack() don't actually operate on
a flat tree anymore; so change the prefix?  How about using of_
instead :-)

[Do you see where this is going? :-) ]


Segher

On Wed, Sep 27, 2006 at 06:52:19PM -0500, Hollis Blanchard wrote:

On Thu, 2006-09-28 at 01:41 +0200, Segher Boessenkool wrote:

quoted

quoted

I talked to David Gibson about this and he pointed out that there is a
problem with using a void * to identify nodes (i.e. returned from
ft_find_device and passed to ft_set_prop etc.).  The problem is that
if we reallocate the space for the tree, then any void * handles that
the user of the library has are then invalid.  Perhaps we have to use
offsets from the beginning of the struct region instead - the offset
for a node will be stable across changes of the node or any of its
descendents, at least.

Or unpack the tree before you operate on it.  You can pack it
again later if you need to pass it as a flat tree again (or the
bootwrapper could implement a real OF client interface, which
is useful for many more things!)

A flat contiguous blob of bytes is convenient to pass around,
but not such a great data structure for basically anything else ;-)

I agree; I think this data structure's design is explicitly static, and
now we're trying to dynamically edit it.

We essentially need to do unpack/pack now whenever the size of the
structure changes (e.g. changing a property size, adding or removing
anything). Making that explicit (and one-time) might be a good idea:
        tree = ft_unpack(blob);
        n = ft_find_node(tree, "/node");
        ft_set_prop(n, "property", value, len);
        ...
        blob = ft_pack(tree);

FWIW, this makes a lot of sense to me.

Mark