On Tue, May 24, 2016 at 05:04:18PM +0300, Pantelis Antoniou wrote:
> Hi David,
>
> > On May 24, 2016, at 13:39 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, May 05, 2016 at 10:48:42PM +0300, Pantelis Antoniou wrote:
> >> This patch enable the generation of symbols & local fixup information
> >> for trees compiled with the -@ (--symbols) option.
> >>
> >> Using this patch labels in the tree and their users emit information
> >> in __symbols__ and __local_fixups__ nodes.
> >>
> >> The __fixups__ node make possible the dynamic resolution of phandle
> >> references which are present in the plugin tree but lie in the
> >> tree that are applying the overlay against.
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> >> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >> Signed-off-by: Jan Luebbe <jlu@xxxxxxxxxxxxxx>
> >> ---
> >> Documentation/manual.txt | 16 ++++
> >> checks.c | 8 +-
> >> dtc-lexer.l | 5 ++
> >> dtc-parser.y | 42 +++++++--
> >> dtc.c | 23 ++++-
> >> dtc.h | 28 +++++-
> >> flattree.c | 2 +-
> >> fstree.c | 2 +-
> >> livetree.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> 9 files changed, 329 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..3e16c24 100644
> >> --- a/Documentation/manual.txt
> >> +++ b/Documentation/manual.txt
> >> @@ -119,6 +119,20 @@ Options:
> >> Make space for <number> reserve map entries
> >> Relevant for dtb and asm output only.
> >>
> >> + -@
> >> + Generates a __symbols__ node at the root node of the resulting blob
> >> + for any node labels used, and for any local references using phandles
> >> + it also generates a __local_fixups__ node that tracks them.
> >> +
> >> + When using the /plugin/ tag all unresolved label references to
> >> + be tracked in the __fixups__ node, making dynamic resolution possible.
> >> +
> >> + -A
> >> + Generate automatically aliases for all node labels. This is similar to
> >> + the -@ option (the __symbols__ node contain identical information) but
> >> + the semantics are slightly different since no phandles are automatically
> >> + generated for labeled nodes.
> >> +
> >> -S <bytes>
> >> Ensure the blob at least <bytes> long, adding additional
> >> space if needed.
> >> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
> >>
> >> devicetree: '/' nodedef
> >>
> >> + plugindecl: '/' 'plugin' '/' ';'
> >
> > You added the new non-terminal definitiom, but didn't use it anywhere.
> >
>
> plugindecl is used by the referenced in versioninfo so it’s used implicitly.
In the actual bison, yes, but not here in the manual - plugindecl is
never referenced. Of course, neither is the version tag, so it's
already a bit wrong.
> >> nodedef: '{' list_of_property list_of_subnode '}' ';'
> >>
> >> property: label PROPNAME '=' propdata ';'
> >> diff --git a/checks.c b/checks.c
> >> index 386f956..3d4c3c6 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
> >>
> >> refnode = get_node_by_ref(dt, m->ref);
> >> if (! refnode) {
> >> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >> - m->ref);
> >> + if (!(tree_get_versionflags(dt) & VF_PLUGIN))
> >> + FAIL(c, "Reference to non-existent node or "
> >> + "label \"%s\"\n", m->ref);
> >> + else /* mark the entry as unresolved */
> >> + *((cell_t *)(prop->val.val + m->offset)) =
> >> + cpu_to_fdt32(0xffffffff);
> >> continue;
> >> }
> >>
> >> diff --git a/dtc-lexer.l b/dtc-lexer.l
> >> index 790fbf6..40bbc87 100644
> >> --- a/dtc-lexer.l
> >> +++ b/dtc-lexer.l
> >> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> >> return DT_V1;
> >> }
> >>
> >> +<*>"/plugin/" {
> >> + DPRINT("Keyword: /plugin/\n");
> >> + return DT_PLUGIN;
> >> + }
> >> +
> >> <*>"/memreserve/" {
> >> DPRINT("Keyword: /memreserve/\n");
> >> BEGIN_DEFAULT();
> >> diff --git a/dtc-parser.y b/dtc-parser.y
> >> index 000873f..2d3399e 100644
> >> --- a/dtc-parser.y
> >> +++ b/dtc-parser.y
> >> @@ -19,6 +19,7 @@
> >> */
> >> %{
> >> #include <stdio.h>
> >> +#include <inttypes.h>
> >>
> >> #include "dtc.h"
> >> #include "srcpos.h"
> >> @@ -52,9 +53,11 @@ extern bool treesource_error;
> >> struct node *nodelist;
> >> struct reserve_info *re;
> >> uint64_t integer;
> >> + unsigned int flags;
> >> }
> >>
> >> %token DT_V1
> >> +%token DT_PLUGIN
> >> %token DT_MEMRESERVE
> >> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> >> %token DT_BITS
> >> @@ -71,6 +74,8 @@ extern bool treesource_error;
> >>
> >> %type <data> propdata
> >> %type <data> propdataprefix
> >> +%type <flags> versioninfo
> >> +%type <flags> plugindecl
> >> %type <re> memreserve
> >> %type <re> memreserves
> >> %type <array> arrayprefix
> >> @@ -101,13 +106,31 @@ extern bool treesource_error;
> >> %%
> >>
> >> sourcefile:
> >> - DT_V1 ';' memreserves devicetree
> >> + versioninfo ';' memreserves devicetree
> >> {
> >> - the_boot_info = build_boot_info($3, $4,
> >> + the_boot_info = build_boot_info($1, $3, $4,
> >> guess_boot_cpuid($4));
> >> }
> >> ;
> >>
> >> +versioninfo:
> >> + DT_V1 plugindecl
> >> + {
> >> + $$ = VF_DT_V1 | $2;
> >> + }
> >> + ;
> >> +
> >> +plugindecl:
> >> + DT_PLUGIN
> >> + {
> >> + $$ = VF_PLUGIN;
> >> + }
> >> + | /* empty */
> >> + {
> >> + $$ = 0;
> >> + }
> >> + ;
> >> +
> >> memreserves:
> >> /* empty */
> >> {
> >> @@ -156,10 +179,14 @@ devicetree:
> >> {
> >> struct node *target = get_node_by_ref($1, $2);
> >>
> >> - if (target)
> >> + if (target) {
> >> merge_nodes(target, $3);
> >> - else
> >> - ERROR(&@2, "Label or path %s not found", $2);
> >> + } else {
> >> + if (symbol_fixup_support)
> >
> > Surely this should depend on the /plugin/ tag, rather than the -@ option..?
> >
>
> the_boot_info is not yet initialized at this point. The only way to access it would
> be to have version flags as a global. Is that OK?
Hmm. So using a global isn't necessarily bad, since the awkward
old-school yacc interface already makes it necessary in some places.
But.. relying on the order in which different yacc semantic actions
are executed (without an explicit data dependency) makes me nervous.
I think the really correct solution is to move the processing of
overlays - for both the dts only and dynamic plugin cases - to a later
pass after parsing. In the non-plugin case it would merge the
subtrees then, for the plugin case it would construct the "meta-tree"
with the fragment/target info.
But, I can see why you wouldn't want to do that right now.
So, in the short term, I think use a global, with a FIXME comment.
> >> + add_orphan_node($1, $3, $2);
> >> + else
> >> + ERROR(&@2, "Label or path %s not found", $2);
> >> + }
> >> $$ = $1;
> >> }
> >> | devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -174,6 +201,11 @@ devicetree:
> >>
> >> $$ = $1;
> >> }
> >> + | /* empty */
> >> + {
> >> + /* build empty node */
> >> + $$ = name_node(build_node(NULL, NULL), "");
> >> + }
> >> ;
> >>
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 5fa23c4..f8277fb 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -31,6 +31,8 @@ int reservenum; /* Number of memory reservation slots */
> >> int minsize; /* Minimum blob size */
> >> int padsize; /* Additional padding to blob */
> >> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> >> +int symbol_fixup_support;
> >> +int auto_label_aliases;
> >>
> >> static void fill_fullpaths(struct node *tree, const char *prefix)
> >> {
> >> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >> #define FDT_VERSION(version) _FDT_VERSION(version)
> >> #define _FDT_VERSION(version) #version
> >> static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
> >> static struct option const usage_long_opts[] = {
> >> {"quiet", no_argument, NULL, 'q'},
> >> {"in-format", a_argument, NULL, 'I'},
> >> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
> >> {"phandle", a_argument, NULL, 'H'},
> >> {"warning", a_argument, NULL, 'W'},
> >> {"error", a_argument, NULL, 'E'},
> >> + {"symbols", no_argument, NULL, '@'},
> >> + {"auto-alias", no_argument, NULL, 'A'},
> >> {"help", no_argument, NULL, 'h'},
> >> {"version", no_argument, NULL, 'v'},
> >> {NULL, no_argument, NULL, 0x0},
> >> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
> >> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> >> "\n\tEnable/disable warnings (prefix with \"no-\")",
> >> "\n\tEnable/disable errors (prefix with \"no-\")",
> >> + "\n\tEnable symbols/fixup support",
> >> + "\n\tEnable auto-alias of labels",
> >> "\n\tPrint this help and exit",
> >> "\n\tPrint version and exit",
> >> NULL,
> >> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
> >> case 'E':
> >> parse_checks_option(false, true, optarg);
> >> break;
> >> -
> >> + case '@':
> >> + symbol_fixup_support = 1;
> >> + break;
> >> + case 'A':
> >> + auto_label_aliases = 1;
> >> + break;
> >> case 'h':
> >> usage(NULL);
> >> default:
> >> @@ -294,6 +305,14 @@ int main(int argc, char *argv[])
> >> if (sort)
> >> sort_tree(bi);
> >>
> >> + if (auto_label_aliases)
> >> + generate_label_tree(bi->dt, "aliases", false);
> >> +
> >> + if (symbol_fixup_support) {
> >> + generate_label_tree(bi->dt, "__symbols__", true);
> >> + generate_fixups_tree(bi->dt);
> >> + }
> >> +
> >
> > These should go before the sort_tree() - since they add nodes, they
> > could result in something that *isn't* sorted when that was requested.
> >
>
> OK
>
> >> if (streq(outname, "-")) {
> >> outf = stdout;
> >> } else {
> >> diff --git a/dtc.h b/dtc.h
> >> index 56212c8..68cb109 100644
> >> --- a/dtc.h
> >> +++ b/dtc.h
> >> @@ -20,7 +20,6 @@
> >> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> >> * USA
> >> */
> >> -
> >
> > Unrelated whitespace change.
> >
>
> OK
>
> >> #include <stdio.h>
> >> #include <string.h>
> >> #include <stdlib.h>
> >> @@ -54,6 +53,12 @@ extern int reservenum; /* Number of memory reservation slots */
> >> extern int minsize; /* Minimum blob size */
> >> extern int padsize; /* Additional padding to blob */
> >> extern int phandle_format; /* Use linux,phandle or phandle properties */
> >> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> >> +extern int auto_label_aliases; /* auto generate labels -> aliases */
> >> +
> >> +/*
> >> + * Tree source globals
> >> + */
> >>
> >> #define PHANDLE_LEGACY 0x1
> >> #define PHANDLE_EPAPR 0x2
> >> @@ -158,6 +163,9 @@ struct node {
> >> int addr_cells, size_cells;
> >>
> >> struct label *labels;
> >> +
> >> + /* only for the root (parent == NULL) */
> >> + struct boot_info *bi;
> >
> > Ugh.. I hate creating circular references unless we really can't avoid
> > them.
> >
>
> Either it’s here, or we pass boot_info to every method.
> Need access to the version flags.
I think passing boot_info down instead of the root node is probably
the right approach here.
>
> >> };
> >>
> >> #define for_each_label_withdel(l0, l) \
> >> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
> >> struct node *name_node(struct node *node, char *name);
> >> struct node *chain_node(struct node *first, struct node *list);
> >> struct node *merge_nodes(struct node *old_node, struct node *new_node);
> >> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
> >>
> >> void add_property(struct node *node, struct property *prop);
> >> void delete_property_by_name(struct node *node, char *name);
> >> @@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> >>
> >>
> >> struct boot_info {
> >> + unsigned int versionflags;
> >> struct reserve_info *reservelist;
> >> struct node *dt; /* the device tree */
> >> uint32_t boot_cpuid_phys;
> >> };
> >>
> >> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> >> +/* version flags definitions */
> >> +#define VF_DT_V1 0x0001 /* /dts-v1/ */
> >
> > Is there any point to this, since dtc doesn't support v0 dts files any
> > more anyway?
> >
>
> Completeness. I can take it out.
Please do.
> >> +#define VF_PLUGIN 0x0002 /* /plugin/ */
> >> +
> >> +static inline unsigned int tree_get_versionflags(struct node *dt)
> >> +{
> >> + if (!dt || !dt->bi)
> >> + return 0;
> >> + return dt->bi->versionflags;
> >> +}
> >> +
> >> +struct boot_info *build_boot_info(unsigned int versionflags,
> >> + struct reserve_info *reservelist,
> >> struct node *tree, uint32_t boot_cpuid_phys);
> >> void sort_tree(struct boot_info *bi);
> >> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
> >> +void generate_fixups_tree(struct node *dt);
> >>
> >> /* Checks */
> >>
> >> diff --git a/flattree.c b/flattree.c
> >> index ec14954..5dde832 100644
> >> --- a/flattree.c
> >> +++ b/flattree.c
> >> @@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
> >>
> >> fclose(f);
> >>
> >> - return build_boot_info(reservelist, tree, boot_cpuid_phys);
> >> + return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
> >> }
> >> diff --git a/fstree.c b/fstree.c
> >> index 6d1beec..54f520b 100644
> >> --- a/fstree.c
> >> +++ b/fstree.c
> >> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
> >> tree = read_fstree(dirname);
> >> tree = name_node(tree, "");
> >>
> >> - return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
> >> + return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
> >> }
> >>
> >> diff --git a/livetree.c b/livetree.c
> >> index e229b84..e7d0fe5 100644
> >> --- a/livetree.c
> >> +++ b/livetree.c
> >> @@ -18,6 +18,7 @@
> >> * USA
> >> */
> >>
> >> +#define _GNU_SOURCE
> >
> > Ugh. I mean, I know the work I did to get dtc working on BSD has
> > bitrotted already, but can we avoid gratuitously introducing
> > non-portability.
> >
>
> It was your suggestion last time, remember? :)
Apparently not :/
> >> #include "dtc.h"
> >>
> >> /*
> >> @@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> >> return old_node;
> >> }
> >>
> >> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> >> +{
> >> + static unsigned int next_orphan_fragment = 0;
> >> + struct node *node = xmalloc(sizeof(*node));
> >> + struct property *p;
> >> + struct data d = empty_data;
> >> + char *name;
> >> + int ret;
> >> +
> >> + memset(node, 0, sizeof(*node));
> >> +
> >> + d = data_add_marker(d, REF_PHANDLE, ref);
> >> + d = data_append_integer(d, 0xffffffff, 32);
> >> +
> >> + p = build_property("target", d);
> >> + add_property(node, p);
> >> +
> >> + ret = asprintf(&name, "fragment@%u",
> >> + next_orphan_fragment++);
> >> + if (ret == -1)
> >> + die("asprintf() failed\n");
> >> + name_node(node, name);
> >> + name_node(new_node, "__overlay__");
> >> +
> >> + add_child(dt, node);
> >> + add_child(node, new_node);
> >> +}
> >
> >
> >
> >
> >> struct node *chain_node(struct node *first, struct node *list)
> >> {
> >> assert(first->next_sibling == NULL);
> >> @@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> >> return list;
> >> }
> >>
> >> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> >> +struct boot_info *build_boot_info(unsigned int versionflags,
> >> + struct reserve_info *reservelist,
> >> struct node *tree, uint32_t boot_cpuid_phys)
> >> {
> >> struct boot_info *bi;
> >>
> >> bi = xmalloc(sizeof(*bi));
> >> + bi->versionflags = versionflags;
> >> bi->reservelist = reservelist;
> >> bi->dt = tree;
> >> bi->boot_cpuid_phys = boot_cpuid_phys;
> >> + /* link back */
> >> + tree->bi = bi;
> >>
> >> return bi;
> >> }
> >> @@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
> >> sort_reserve_entries(bi);
> >> sort_node(bi->dt);
> >> }
> >> +
> >> +/* utility helper to avoid code duplication */
> >> +static struct node *build_and_name_child_node(struct node *parent, char *name)
> >> +{
> >> + struct node *node;
> >> +
> >> + node = build_node(NULL, NULL);
> >> + name_node(node, xstrdup(name));
> >> + add_child(parent, node);
> >> +
> >> + return node;
> >> +}
> >> +
> >> +static void generate_label_tree_internal(struct node *dt, struct node *node,
> >> + char *gen_node_name, bool allocph)
> >> +{
> >> + struct node *c, *an;
> >> + struct property *p;
> >> + struct label *l;
> >> +
> >> + /* if if there are labels */
> >> + if (node->labels) {
> >> + /* an is the aliases/__symbols__ node */
> >> + an = get_subnode(dt, gen_node_name);
> >
> > You can factor this out to the caller and just pass 'an' in.
> >
>
> OK
>
> >> + /* if no node exists, create it */
> >
> > Comment does not match the code
> >
>
> Ugh.
>
> >> + if (!an)
> >> + die("Could not get label node\n");
> >> +
> >> + /* now add the label in the node */
> >> + for_each_label(node->labels, l) {
> >> + /* check whether the label already exists */
> >> + p = get_property(an, l->label);
> >> + if (p) {
> >> + fprintf(stderr, "WARNING: label %s already"
> >> + " exists in /%s", l->label,
> >> + gen_node_name);
> >> + continue;
> >> + }
> >> +
> >> + /* insert it */
> >> + p = build_property(l->label,
> >> + data_copy_escape_string(node->fullpath,
> >> + strlen(node->fullpath)));
> >> + add_property(an, p);
> >> + }
> >> +
> >> + /* force allocation of a phandle for this node */
> >> + if (allocph)
> >> + (void)get_node_phandle(dt, node);
> >> + }
> >> +
> >> + for_each_child(node, c)
> >> + generate_label_tree_internal(dt, c, gen_node_name, allocph);
> >> +}
> >> +
> >> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> >> +{
> >> + build_and_name_child_node(dt, gen_node_name);
> >> + generate_label_tree_internal(dt, dt, gen_node_name, allocph);
> >> +}
> >> +
> >> +static char *fixups_name = "__fixups__";
> >> +static char *local_fixups_name = "__local_fixups__";
> >> +
> >> +static void add_fixup_entry(struct node *dt, struct node *node,
> >> + struct property *prop, struct marker *m)
> >> +{
> >> + struct node *fn; /* local fixup node */
> >
> > Comment is misleading since this is __fixups__ rather than
> > __local_fixups__.
> >
>
> OK
>
> >> + struct property *p;
> >> + struct data d;
> >> + char *entry;
> >> + int ret;
> >> +
> >> + /* m->ref can only be a REF_PHANDLE, but check anyway */
> >> + if (m->type != REF_PHANDLE)
> >> + die("Fixup entry can only be a ref to a phandle\n");
> >
> > If it's not a REF_PHANDLE, that indicates a bug elsewhere, yes? In
> > which case assert() would be more appropriate.
> >
>
> OK
>
> >> + /* fn is the node we're putting entries in */
> >> + fn = get_subnode(dt, fixups_name);
> >> + if (!fn)
> >> + die("Can't get fixups node\n");
> >
> > Again, assert() would be better.
> >
>
> OK
>
> >> + /* there shouldn't be any ':' in the arguments */
> >> + if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> >> + die("arguments should not contain ':'\n");
> >> +
> >> + ret = asprintf(&entry, "%s:%s:%u",
> >> + node->fullpath, prop->name, m->offset);
> >> + if (ret == -1)
> >> + die("asprintf() failed\n");
> >
> > Looks like we really should put in an xasprintf() utility function,
> > both to address the BSD portability, and avoid clunky checks for
> > allocation failures.
> >
>
> No worries, I’ll add it in a separate patch.
>
> >> + p = get_property(fn, m->ref);
> >> + if (p) {
> >> + d = data_append_data(p->val, entry, strlen(entry) + 1);
> >> + p->val = d;
> >> + } else {
> >> + d = data_append_data(empty_data, entry, strlen(entry) + 1);
> >> + add_property(fn, build_property(m->ref, d));
> >> + }
> >> +}
> >> +
> >> +static void add_local_fixup_entry(struct node *dt, struct node *node,
> >> + struct property *prop, struct marker *m,
> >> + struct node *refnode)
> >> +{
> >> + struct node *lfn, *wn, *nwn; /* local fixup node, walk node, new */
> >> + struct property *p;
> >> + struct data d;
> >> + char *s, *e, *comp;
> >> + int len;
> >> +
> >> + /* fn is the node we're putting entries in */
> >> + lfn = get_subnode(dt, local_fixups_name);
> >> + if (!lfn)
> >> + die("Can't get local fixups node\n");
> >
> > Again, assert().
> >
> >> + /* walk the path components creating nodes if they don't exist */
> >> + comp = xmalloc(strlen(node->fullpath) + 1);
> >> + /* start skipping the first / */
> >> + s = node->fullpath + 1;
> >> + wn = lfn;
> >> + while (*s) {
> >
> > Given the break; below is the loop condition redundant?
> >
>
> Sanity check if fullpath is empty. Could assert instead.
I think assert() makes more sense. The loop condition still won't
actually make this code do anything useful in that case.
> >> + /* retrieve path component */
> >> + e = strchr(s, '/');
> >> + if (e == NULL)
> >> + e = s + strlen(s);
> >> + len = e - s;
> >> + memcpy(comp, s, len);
> >> + comp[len] = '\0';
> >> +
> >> + /* if no node exists, create it */
> >> + nwn = get_subnode(wn, comp);
> >> + if (!nwn)
> >> + nwn = build_and_name_child_node(wn, comp);
> >> + wn = nwn;
> >> +
> >> + /* last path component */
> >> + if (!*e)
> >> + break;
> >> +
> >> + /* next path component */
> >> + s = e + 1;
> >> + }
> >> + free(comp);
> >> +
> >> + p = get_property(wn, prop->name);
> >> + d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
> >> + if (!p)
> >> + add_property(wn, build_property(prop->name, d));
> >> + else
> >> + p->val = d;
> >
> > Hmm.. am append_to_property helper would simplify both this and the
> > other fixup function.
> >
>
> OK
>
> >> +}
> >> +
> >> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
> >> +{
> >> + struct node *c;
> >> + struct property *prop;
> >> + struct marker *m;
> >> + struct node *refnode;
> >> +
> >> + for_each_property(node, prop) {
> >> + m = prop->val.markers;
> >> + for_each_marker_of_type(m, REF_PHANDLE) {
> >> + refnode = get_node_by_ref(dt, m->ref);
> >> + if (!refnode)
> >> + add_fixup_entry(dt, node, prop, m);
> >> + else
> >> + add_local_fixup_entry(dt, node, prop, m,
> >> + refnode);
> >> + }
> >> + }
> >> +
> >> + for_each_child(node, c)
> >> + generate_fixups_tree_internal(dt, c);
> >> +}
> >> +
> >> +void generate_fixups_tree(struct node *dt)
> >> +{
> >> + build_and_name_child_node(dt, fixups_name);
> >> + build_and_name_child_node(dt, local_fixups_name);
> >> + generate_fixups_tree_internal(dt, dt);
> >> +}
> >
>
--
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
Attachment:
signature.asc
Description: PGP signature
