Series short description

yyerror is meant to be called by the parser internal code, and it's interface is limited. Instead create and call a new error message routine that allows formatted strings to be used.

yyerror uses the new routine so error formatting remains consistent.

Signed-of-by John Bonesio <bones-s3s/[email protected]>

---

 dtc-parser.y |   28 ++++++++++++++++++----------
 srcpos.c     |   21 +++++++++++++--------
 srcpos.h     |    2 ++
 3 files changed, 33 insertions(+), 18 deletions(-)

dtc-parser.y +18 -10

index 0aaf8e8..b58ba8e 100644
@@ -27,6 +27,7 @@
 YYLTYPE yylloc;
 
 extern int yylex(void);
+extern void print_error(char const *fmt, ...);
 extern void yyerror(char const *s);
 
 extern struct boot_info *the_boot_info;

@@ -136,8 +137,7 @@ devicetree:
 			if (target)
 				merge_nodes(target, $3);
 			else
-				yyerror("label does not exist in "
-					" node redefinition");
+				print_error("label, '%s' not found", $2);
 			$$ = $1;
 		}
 	;

@@ -200,8 +200,7 @@ propdata:
 
 			if ($6 != 0)
 				if (fseek(f, $6, SEEK_SET) != 0)
-					srcpos_error(&yylloc,
-						     "Couldn't seek to offset %llu in \"%s\": %s",
+					print_error("Couldn't seek to offset %llu in \"%s\": %s",
 						     (unsigned long long)$6,
 						     $4.val,
 						     strerror(errno));

@@ -295,7 +294,7 @@ subnodes:
 		}
 	| subnode propdef
 		{
-			yyerror("syntax error: properties must precede subnodes");
+			print_error("syntax error: properties must precede subnodes");
 			YYERROR;
 		}
 	;

@@ -314,12 +313,21 @@ subnode:
 
 %%
 
-void yyerror(char const *s)
+void print_error(char const *fmt, ...)
 {
-	srcpos_error(&yylloc, "%s", s);
+	va_list va;
+
+	va_start(va, fmt);
+	srcpos_verror(&yylloc, fmt, va);
+	va_end(va);
+
 	treesource_error = 1;
 }
 
+void yyerror(char const *s) {
+	print_error("%s", s);
+}
+
 static unsigned long long eval_literal(const char *s, int base, int bits)
 {
 	unsigned long long val;

@@ -328,11 +336,11 @@ static unsigned long long eval_literal(const char *s, int base, int bits)
 	errno = 0;
 	val = strtoull(s, &e, base);
 	if (*e)
-		yyerror("bad characters in literal");
+		print_error("bad characters in literal");
 	else if ((errno == ERANGE)
 		 || ((bits < 64) && (val >= (1ULL << bits))))
-		yyerror("literal out of range");
+		print_error("literal out of range");
 	else if (errno != 0)
-		yyerror("bad literal");
+		print_error("bad literal");
 	return val;
 }

srcpos.c +13 -8

index 87d7f17..2dbc874 100644
@@ -208,20 +208,25 @@ srcpos_string(struct srcpos *pos)
 	return pos_str;
 }
 
+void
+srcpos_verror(struct srcpos *pos, char const *fmt, va_list va)
+{
+       const char *srcstr;
+
+       srcstr = srcpos_string(pos);
+
+       fprintf(stdout, "Error: %s ", srcstr);
+       vfprintf(stdout, fmt, va);
+       fprintf(stdout, "\n");
+}
 
 void
 srcpos_error(struct srcpos *pos, char const *fmt, ...)
 {
-	const char *srcstr;
 	va_list va;
-	va_start(va, fmt);
-
-	srcstr = srcpos_string(pos);
-
-	fprintf(stderr, "Error: %s ", srcstr);
-	vfprintf(stderr, fmt, va);
-	fprintf(stderr, "\n");
 
+	va_start(va, fmt);
+	srcpos_verror(pos, fmt, va);
 	va_end(va);
 }
 

srcpos.h +2 -0

index 985f847..bd7966e 100644
@@ -76,6 +76,8 @@ extern struct srcpos *srcpos_copy(struct srcpos *pos);
 extern char *srcpos_string(struct srcpos *pos);
 extern void srcpos_dump(struct srcpos *pos);
 
+extern void srcpos_verror(struct srcpos *pos, char const *, va_list va)
+     __attribute__((format(printf, 2, 0)));
 extern void srcpos_error(struct srcpos *pos, char const *, ...)
      __attribute__((format(printf, 2, 3)));
 extern void srcpos_warn(struct srcpos *pos, char const *, ...)

Quoted 6 lines

yyerror is meant to be called by the parser internal code, and it's interface is limited. Instead create and call a new error message routine that allows formatted strings to be used.

yyerror uses the new routine so error formatting remains consistent.

Quoted 1 line

Signed-of-by John Bonesio <bones-s3s/[email protected]>

Picked up and pushed out to my git tree. Jon is a little tied up for the moment, so I'll maintain a working tree at
git://git.secretlab.ca/git/dtc.git until we've got this work finished and we can ask him to merge it.

g.

This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.

The current sytem allows an existing node to be extended with new properties and subnodes.

The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

---

 dtc-lexer.l  |    6 ++++++
 dtc-parser.y |   11 +++++++++++
 dtc.h        |    1 +
 livetree.c   |   18 ++++++++++++++++++
 4 files changed, 36 insertions(+), 0 deletions(-)

dtc-lexer.l +6 -0

index 081e13a..80a886a 100644
@@ -96,6 +96,12 @@ static int pop_input_file(void);
 			return DT_MEMRESERVE;
 		}
 
+<*>"/remove-node/"	{
+			DPRINT("Keyword: /remove-node/\n");
+			BEGIN_DEFAULT();
+			return DT_REMOVENODE;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);

dtc-parser.y +11 -0

index b58ba8e..1ba0478 100644
@@ -55,6 +55,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 
 %token DT_V1
 %token DT_MEMRESERVE
+%token DT_REMOVENODE
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE

@@ -140,6 +141,16 @@ devicetree:
 				print_error("label, '%s' not found", $2);
 			$$ = $1;
 		}
+	| devicetree DT_REMOVENODE DT_REF ';'
+		{
+			struct node *target;
+
+			target = get_node_by_label($1, $3);
+			if (target)
+				remove_child(target->parent, target);
+			else
+				print_error("label, '%s' not found", $3);
+		}
 	;
 
 nodedef:

dtc.h +1 -0

index b36ac5d..a7f3667 100644
@@ -178,6 +178,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
+void remove_child(struct node *parent, struct node *child);
 
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);

livetree.c +18 -0

index 13c5f10..21a4c48 100644
@@ -202,6 +202,24 @@ void add_child(struct node *parent, struct node *child)
 	*p = child;
 }
 
+void remove_child(struct node *parent, struct node *child)
+{
+	struct node **p;
+
+	/* Make sure we've got a consistent tree here */
+	assert(child->parent == parent);
+
+	p = &parent->children;
+	while (*p) {
+		if (*p == child) {
+			*p = (*p)->next_sibling;
+			break;
+		}
+		p = &((*p)->next_sibling);
+	}
+	child->parent = NULL;
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));

This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.

The current sytem allows an existing node to be extended with new properties and subnodes.

The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.

dtc-parser.y +0 -0

 	;
 
 nodedef:

dtc.h +1 -0

index b36ac5d..a7f3667 100644
@@ -178,6 +178,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
+void remove_child(struct node *parent, struct node *child);
 
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);

livetree.c +18 -0

index 13c5f10..21a4c48 100644
@@ -202,6 +202,24 @@ void add_child(struct node *parent, struct node *child)
 	*p = child;
 }
 
+void remove_child(struct node *parent, struct node *child)
+{
+	struct node **p;
+
+	/* Make sure we've got a consistent tree here */
+	assert(child->parent == parent);
+
+	p = &parent->children;
+	while (*p) {
+		if (*p == child) {
+			*p = (*p)->next_sibling;
+			break;
+		}
+		p = &((*p)->next_sibling);
+	}
+	child->parent = NULL;
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));

Changes to allow us to specify a node by it's path. A path can be used in place of a label.

This is particularly useful when overriding existing nodes.
This way we don't have to label every possible node in a device tree we know is a base device tree for a class of systems, and we know the tree will be modified for the specific systems.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

---

 dtc-lexer.l  |   26 ++++++++++++++++++++++----
 dtc-parser.y |   10 ++++------
 livetree.c   |   20 +++++++++++++++++++-
 3 files changed, 45 insertions(+), 11 deletions(-)

dtc-lexer.l +22 -4

index 80a886a..216a3d2 100644
@@ -115,13 +115,31 @@ static int pop_input_file(void);
 			return DT_LITERAL;
 		}
 
-<*>\&{LABEL}	{	/* label reference */
-			DPRINT("Ref: %s\n", yytext+1);
-			yylval.labelref = xstrdup(yytext+1);
+
+<*>\&{LABEL}    {       /* label reference without the braces*/
+                        DPRINT("Ref: %s\n", yytext+1);
+                        yylval.labelref = xstrdup(yytext+1);
+                        return DT_REF;
+		}
+
+<*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */
+			/*
+			 * Possibly this could be parsed by the parser rather
+			 * than as a lexical element.
+			 *
+			 * What is intended here is to support the following
+			 * type of references:
+			 * a) &{/path/to/the/node/reference}
+			 * b) &{label}
+			 * c) &{label/path/from/labeled/node/to/reference}
+			 */
+			yytext[yyleng-1] = '\0';
+			DPRINT("Ref: %s\n", yytext+2);
+			yylval.labelref = xstrdup(yytext+2);
 			return DT_REF;
 		}
 
-"&{/"{PATHCHAR}+\}	{	/* new-style path reference */
+<*>"&{/"{PATHCHAR}+\}	{	/* new-style path reference (with braces) */
 			yytext[yyleng-1] = '\0';
 			DPRINT("Ref: %s\n", yytext+2);
 			yylval.labelref = xstrdup(yytext+2);

dtc-parser.y +0 -0

index 1ba0478..0a74c86 100644
@@ -132,24 +132,22 @@ devicetree:
 		}
 	| devicetree DT_REF nodedef

dtc-parser.y +2 -3

 			$$ = $1;
 		}
 	| devicetree DT_REMOVENODE DT_REF ';'
 		{
-			struct node *target;
+			struct node *target = get_node_by_ref($1, $3);
 
-			target = get_node_by_label($1, $3);
 			if (target)
 				remove_child(target->parent, target);
 			else
-				print_error("label, '%s' not found", $3);
+				print_error("label or path, '%s', not found", $3);
 		}
 	;
 

livetree.c +19 -1

index 21a4c48..bf8796b 100644
@@ -429,10 +429,28 @@ struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
 
 struct node *get_node_by_ref(struct node *tree, const char *ref)
 {
+	struct node *node;
+
 	if (ref[0] == '/')
 		return get_node_by_path(tree, ref);
-	else
+	else {
+		/*
+		 * Support finding a node reference that is rooted by
+		 * a label reference.
+		 *
+		 * References rooted by a label have a '/' in them.
+		 */
+		char *p = strchr(ref, '/');
+
+		if (p) {
+			*p = '\0';
+			node = get_node_by_label(tree, ref);
+			*p = '/';
+			return get_node_by_path(node, p+1);
+		}
+
 		return get_node_by_label(tree, ref);
+	}
 }
 
 cell_t get_node_phandle(struct node *root, struct node *node)

Changes to allow us to specify a node by it's path. A path can be used in place of a label.

This is particularly useful when overriding existing nodes.
This way we don't have to label every possible node in a device tree we know is a base device tree for a class of systems, and we know the tree will be modified for the specific systems.

livetree.c +2 -0

index 21a4c48..bf8796b 100644
@@ -429,10 +429,28 @@ struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
 
 struct node *get_node_by_ref(struct node *tree, const char *ref)
 {
+	struct node *node;
+
 	if (ref[0] == '/')

livetree.c +6 -0

+			node = get_node_by_label(tree, ref);
+			*p = '/';
+			return get_node_by_path(node, p+1);
+		}
+
 		return get_node_by_label(tree, ref);
+	}
 }
 
 cell_t get_node_phandle(struct node *root, struct node *node)

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

---

 dtc-lexer.l  |   14 ++++++++++----
 dtc-parser.y |    6 ++++++
 dtc.h        |    7 +++++++
 flattree.c   |    3 +++
 livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
 5 files changed, 59 insertions(+), 9 deletions(-)

dtc-lexer.l +0 -0

index 216a3d2..efa89b4 100644

dtc-lexer.l +0 -0

 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);

@@ -116,10 +122,10 @@ static int pop_input_file(void);
 		}
 

dtc-lexer.l +0 -0

 <*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */

dtc-parser.y +1 -0

index 0a74c86..ac9cfd7 100644
@@ -56,6 +56,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 %token DT_V1
 %token DT_MEMRESERVE
 %token DT_REMOVENODE
+%token DT_UNDEFINED
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE

dtc-parser.y +1 -0

+		}
 
 	| DT_LABEL propdef
 		{

dtc-parser.y +0 -0

 			add_label(&$2->labels, $1);

dtc.h +0 -0

index a7f3667..b3fca6e 100644

dtc.h +5 -0

+
 struct property {
+	int undefined;  /* if the property is set to undefined, this feild is
+	                   set to PROP_UNDEFINED */
 	char *name;
 	struct data val;
 

@@ -170,6 +175,7 @@ void add_label(struct label **labels, char *label);
 struct property *build_property(char *name, struct data val);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
+void undefine_property(struct property *prop);
 
 struct node *build_node(struct property *proplist, struct node *children);
 struct node *name_node(struct node *node, char *name);

@@ -177,6 +183,7 @@ struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
+void remove_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
 void remove_child(struct node *parent, struct node *child);
 

flattree.c +3 -0

index ead0332..00439e9 100644
@@ -275,6 +275,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 	for_each_property(tree, prop) {
 		int nameoff;
 
+		if (prop->undefined)
+			continue;
+
 		if (streq(prop->name, "name"))
 			seen_name_prop = 1;
 

livetree.c +15 -5

index bf8796b..2ef734d 100644
@@ -51,6 +51,11 @@ struct property *build_property(char *name, struct data val)
 	return new;
 }
 
+void undefine_property(struct property *prop)
+{
+	prop->undefined = 1;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);

@@ -121,11 +126,15 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 		/* Look for a collision, set new value if there is */
 		for_each_property(old_node, old_prop) {
 			if (streq(old_prop->name, new_prop->name)) {
-				/* Add new labels to old property */
-				for_each_label(new_prop->labels, l)
-					add_label(&old_prop->labels, l->label);
-
-				old_prop->val = new_prop->val;
+				if (new_prop->undefined) {
+					remove_property(old_node, old_prop);
+				} else {
+					/* Add new labels to old property */
+					for_each_label(new_prop->labels, l)
+						add_label(&old_prop->labels, l->label);
+
+					old_prop->val = new_prop->val;
+				}
 				free(new_prop);
 				new_prop = NULL;
 				break;

@@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
 	*p = prop;
 }
 
+void remove_property(struct node *node, struct property *prop)

livetree.c +8 -0

+		}
+		p = &((*p)->next);
+	}
+	/* property not in the node? it's probably an error, so flag it. */
+	assert(found);
+}
+
+
 void add_child(struct node *parent, struct node *child)
 {
 	struct node **p;

Quoted 7 lines · John Bonesio

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

Implementation looks good.

Quoted 17 lines

---

 dtc-lexer.l  |   14 ++++++++++----
 dtc-parser.y |    6 ++++++
 dtc.h        |    7 +++++++
 flattree.c   |    3 +++
 livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
 5 files changed, 59 insertions(+), 9 deletions(-)

dtc-lexer.l +0 -0

index 216a3d2..efa89b4 100644
@@ -102,6 +102,12 @@ static int pop_input_file(void);
 			return DT_REMOVENODE;
 		}
 

dtc-lexer.l static int pop_input_file(void); · line 110

Quoted 6 lines

dtc-lexer.l +6 -0

+<*>"/undef-prop/"	{
+			DPRINT("Keyword: /undef-prop/\n");
+			BEGIN_DEFAULT();
+			return DT_UNDEFINED;
+		}
+

Does /undef-prop/ really need to be using <*> to match in all start conditions?

dtc-lexer.l propdef: · line 182

Quoted 5 lines

dtc-lexer.l +1 -0

@@ -178,6 +179,11 @@ propdef:
 		{
 			$$ = build_property($1, empty_data);
 		}
+	| DT_PROPNODENAME '=' DT_UNDEFINED ';'

Hmmm. I'm going to make this comment once, but I'll shut-up if you guys disagree with me because the details have already been hashed out several times, and I've already said I'd be okay with the above form.

The more I look at it, the more I prefer the form

	/undef-prop/ property;
instead of
	property = /undef-prop/;

The reason being is that while the assignment form does work, it isn't a very natural construct. Removal is not logically the same as assignment. /undef-prop/ is something that is performed on a property. Syntax that shows /undef-prop/ being assigned as a property value doesn't ring true for me as the right thing to do.

So, my vote is for the "/undef-prop/ property;" form, but I hold my piece if you both disagree with me.

Quoted 27 lines · Grant Likely

Quoted 7 lines · John Bonesio

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

Implementation looks good.

Quoted 17 lines

---

 dtc-lexer.l  |   14 ++++++++++----
 dtc-parser.y |    6 ++++++
 dtc.h        |    7 +++++++
 flattree.c   |    3 +++
 livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
 5 files changed, 59 insertions(+), 9 deletions(-)

dtc-lexer.l +0 -0

index 216a3d2..efa89b4 100644
@@ -102,6 +102,12 @@ static int pop_input_file(void);
 			return DT_REMOVENODE;
 		}
 

Quoted 9 lines

dtc-lexer.l static int pop_input_file(void); · line 110

Quoted 6 lines

dtc-lexer.l +6 -0

+<*>"/undef-prop/"	{
+			DPRINT("Keyword: /undef-prop/\n");
+			BEGIN_DEFAULT();
+			return DT_UNDEFINED;
+		}
+

Does /undef-prop/ really need to be using <*> to match in all start conditions?

It doesn't need to, but it's a good idea for it to do so, because if the keyword is lexed as a keyword everywhere, it will lead to more meaningful error messages if it's put somewhere it shouldn't be.

In fact, something I've learnt writing dtc is that in general you should make your lexical tokens as wide as they can without colliding with each other, then check that they have the right contents later. That way you get a clear error message from the checking code ("such-and-so contained an illegal character"), rather than the lexer breaking it into different tokens instead and the parser generating some cryptic error.

[snip]

Quoted 15 lines

dtc-lexer.l propdef: · line 182

Quoted 5 lines

dtc-lexer.l +1 -0

@@ -178,6 +179,11 @@ propdef:
 		{
 			$$ = build_property($1, empty_data);
 		}
+	| DT_PROPNODENAME '=' DT_UNDEFINED ';'

Hmmm. I'm going to make this comment once, but I'll shut-up if you guys disagree with me because the details have already been hashed out several times, and I've already said I'd be okay with the above form.

The more I look at it, the more I prefer the form

	/undef-prop/ property;
instead of
	property = /undef-prop/;

Quoted 8 lines

The reason being is that while the assignment form does work, it isn't a very natural construct. Removal is not logically the same as assignment. /undef-prop/ is something that is performed on a property. Syntax that shows /undef-prop/ being assigned as a property value doesn't ring true for me as the right thing to do.

So, my vote is for the "/undef-prop/ property;" form, but I hold my piece if you both disagree with me.

I.. yeah.. I'm not sure. I was leaning towards prop = /undef-prop/; before, but you've more-or-less persuaded me here.

Quoted 2 lines · David Gibson

Quoted 1 line · Grant Likely

On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:

Quoted 7 lines

Quoted 2 lines

Does /undef-prop/ really need to be using <*> to match in all start conditions?

It doesn't need to, but it's a good idea for it to do so, because if the keyword is lexed as a keyword everywhere, it will lead to more meaningful error messages if it's put somewhere it shouldn't be.

Quoted 7 lines

In fact, something I've learnt writing dtc is that in general you should make your lexical tokens as wide as they can without colliding with each other, then check that they have the right contents later. That way you get a clear error message from the checking code ("such-and-so contained an illegal character"), rather than the lexer breaking it into different tokens instead and the parser generating some cryptic error.

This is probably what David is saying. Generally you want the lexer to be context free - meaning everything gets tokenized the same way everywhere.

The fact that we're using various start conditions in the dtc, is a little unsettling to me. It makes me wonder if we've got functionality in the lexer that really should be in the parser.

Quoted 7 lines · John Bonesio

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

Signed-off-by John Bonesio <bones-s3s/[email protected]>

Implementation looks good.

Quoted 14 lines

---

 dtc-lexer.l  |   14 ++++++++++----
 dtc-parser.y |    6 ++++++
 dtc.h        |    7 +++++++
 flattree.c   |    3 +++
 livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
 5 files changed, 59 insertions(+), 9 deletions(-)

dtc-lexer.l +0 -0

index 216a3d2..efa89b4 100644

dtc-lexer.l static int pop_input_file(void); · line 110

Quoted 9 lines

dtc-lexer.l +6 -0

@@ -102,6 +102,12 @@ static int pop_input_file(void);
 			return DT_REMOVENODE;
 		}
 
+<*>"/undef-prop/"	{
+			DPRINT("Keyword: /undef-prop/\n");
+			BEGIN_DEFAULT();
+			return DT_UNDEFINED;
+		}
+

Does /undef-prop/ really need to be using <*> to match in all start conditions?

Quoted 6 lines

dtc-lexer.l +0 -0

 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);

@@ -116,10 +122,10 @@ static int pop_input_file(void);
 		}
 

dtc-lexer.l static int pop_input_file(void); · line 129

Quoted 10 lines

dtc-lexer.l +4 -4

 
-<*>\&{LABEL}    {       /* label reference without the braces*/
-                        DPRINT("Ref: %s\n", yytext+1);
-                        yylval.labelref = xstrdup(yytext+1);
-                        return DT_REF;
+<*>\&{LABEL}	{       /* label reference without the braces*/
+			DPRINT("Ref: %s\n", yytext+1);
+			yylval.labelref = xstrdup(yytext+1);
+	    		return DT_REF;
 		}

Unrelated whitespace change. In general, patches should avoid making unrelated changes in the same patch, even if they are correct, because they decrease the signal-to-noise ratio for patch reviewers. Whitespace changes are particularly offensive because the can end up masking (to a reviewer) functional changes in the same block.

Quoted 15 lines

dtc-lexer.l +0 -0

 <*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */

dtc-parser.y +1 -0

index 0a74c86..ac9cfd7 100644
@@ -56,6 +56,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 %token DT_V1
 %token DT_MEMRESERVE
 %token DT_REMOVENODE
+%token DT_UNDEFINED
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE

dtc-parser.y propdef: · line 182

Quoted 4 lines

dtc-parser.y +1 -0

@@ -178,6 +179,11 @@ propdef:
 		{
 			$$ = build_property($1, empty_data);
 		}
+	| DT_PROPNODENAME '=' DT_UNDEFINED ';'

Hmmm. I'm going to make this comment once, but I'll shut-up if you guys disagree with me because the details have already been hashed out several times, and I've already said I'd be okay with the above form.

The more I look at it, the more I prefer the form

	/undef-prop/ property;
instead of
	property = /undef-prop/;

The reason being is that while the assignment form does work, it isn't a very natural construct. Removal is not logically the same as assignment. /undef-prop/ is something that is performed on a property. Syntax that shows /undef-prop/ being assigned as a property value doesn't ring true for me as the right thing to do.

So, my vote is for the "/undef-prop/ property;" form, but I hold my piece if you both disagree with me.

dtc-parser.y propdef: · line 185

Quoted 3 lines

dtc-parser.y +3 -0

+		{
+			$$ = build_property($1, empty_data);
+			undefine_property($$)

Don't bother with the undefine_property function. Just do:

			$$->undefined = 1;

Quoted 10 lines

dtc-parser.y +1 -0

+		}
 
 	| DT_LABEL propdef
 		{

dtc-parser.y +0 -0

 			add_label(&$2->labels, $1);

dtc.h +0 -0

index a7f3667..b3fca6e 100644

dtc.h struct label { · line 134

Quoted 5 lines

dtc.h +2 -0

@@ -130,7 +130,12 @@ struct label {
 	struct label *next;
 };
 
+#define PROP_DEFINED (0)
+#define PROP_UNDEFINED (1)

No need for the #defines. Just use 0 and 1 in the code when working with boolean flags. Besides, these #defines aren't used anywhere anyway. :-)

Quoted 79 lines

dtc.h +5 -0

+
 struct property {
+	int undefined;  /* if the property is set to undefined, this feild is
+	                   set to PROP_UNDEFINED */
 	char *name;
 	struct data val;
 

@@ -170,6 +175,7 @@ void add_label(struct label **labels, char *label);
 struct property *build_property(char *name, struct data val);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
+void undefine_property(struct property *prop);
 
 struct node *build_node(struct property *proplist, struct node *children);
 struct node *name_node(struct node *node, char *name);

@@ -177,6 +183,7 @@ struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
+void remove_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
 void remove_child(struct node *parent, struct node *child);
 

flattree.c +3 -0

index ead0332..00439e9 100644
@@ -275,6 +275,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 	for_each_property(tree, prop) {
 		int nameoff;
 
+		if (prop->undefined)
+			continue;
+
 		if (streq(prop->name, "name"))
 			seen_name_prop = 1;
 

livetree.c +15 -5

index bf8796b..2ef734d 100644
@@ -51,6 +51,11 @@ struct property *build_property(char *name, struct data val)
 	return new;
 }
 
+void undefine_property(struct property *prop)
+{
+	prop->undefined = 1;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);

@@ -121,11 +126,15 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 		/* Look for a collision, set new value if there is */
 		for_each_property(old_node, old_prop) {
 			if (streq(old_prop->name, new_prop->name)) {
-				/* Add new labels to old property */
-				for_each_label(new_prop->labels, l)
-					add_label(&old_prop->labels, l->label);
-
-				old_prop->val = new_prop->val;
+				if (new_prop->undefined) {
+					remove_property(old_node, old_prop);
+				} else {
+					/* Add new labels to old property */
+					for_each_label(new_prop->labels, l)
+						add_label(&old_prop->labels, l->label);
+
+					old_prop->val = new_prop->val;
+				}
 				free(new_prop);
 				new_prop = NULL;
 				break;

@@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
 	*p = prop;
 }
 
+void remove_property(struct node *node, struct property *prop)

livetree.c void add_property(struct node *node, struct property *prop) · line 210

Quoted 10 lines

livetree.c +10 -0

+{
+	struct property **p;
+	int found = 0;
+
+	p = &node->proplist;
+	while (*p) {
+		if (*p == prop) {
+			*p = (*p)->next;
+			found = 1;
+			break;

You could just return at this point, and assert unconditionally if the loop exits. That would be slightly more concise.

Quoted 12 lines

livetree.c +8 -0

+		}
+		p = &((*p)->next);
+	}
+	/* property not in the node? it's probably an error, so flag it. */
+	assert(found);
+}
+
+
 void add_child(struct node *parent, struct node *child)
 {
 	struct node **p;

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

dtc.h +5 -0

+
 struct property {
+	int undefined;  /* if the property is set to undefined, this feild is
+	                   set to PROP_UNDEFINED */
 	char *name;
 	struct data val;
 

@@ -170,6 +175,7 @@ void add_label(struct label **labels, char *label);
 struct property *build_property(char *name, struct data val);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
+void undefine_property(struct property *prop);
 
 struct node *build_node(struct property *proplist, struct node *children);
 struct node *name_node(struct node *node, char *name);

@@ -177,6 +183,7 @@ struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
+void remove_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
 void remove_child(struct node *parent, struct node *child);
 

flattree.c +3 -0

index ead0332..00439e9 100644
@@ -275,6 +275,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 	for_each_property(tree, prop) {
 		int nameoff;
 
+		if (prop->undefined)
+			continue;
+
 		if (streq(prop->name, "name"))
 			seen_name_prop = 1;
 

livetree.c +5 -0

index bf8796b..2ef734d 100644
@@ -51,6 +51,11 @@ struct property *build_property(char *name, struct data val)
 	return new;
 }
 
+void undefine_property(struct property *prop)
+{
+	prop->undefined = 1;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);

@@ -121,11 +126,15 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 		/* Look for a collision, set new value if there is */

livetree.c +7 -0

+				} else {
+					/* Add new labels to old property */
+					for_each_label(new_prop->labels, l)
+						add_label(&old_prop->labels, l->label);
+
+					old_prop->val = new_prop->val;
+				}
 				free(new_prop);
 				new_prop = NULL;

livetree.c +6 -0

 				break;

@@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
 	*p = prop;
 }
 
+void remove_property(struct node *node, struct property *prop)
+{
+	struct property **p;
+	int found = 0;
+
+	p = &node->proplist;

livetree.c +3 -0

+}
+
+
 void add_child(struct node *parent, struct node *child)
 {
 	struct node **p;

Quoted 5 lines · John Bonesio

When updating existing nodes in a device tree merge operation, properties can be removed by setting the value to /undef-prop/.

if /undef-prop/ is assigned to a property that doesn't exist, the property is treated the same as if it had not been declared.

[snip]

dtc.h struct label { · line 134

Quoted 10 lines

dtc.h +2 -0

index a7f3667..b3fca6e 100644
@@ -130,7 +130,12 @@ struct label {
 	struct label *next;
 };
 
+#define PROP_DEFINED (0)
+#define PROP_UNDEFINED (1)

You never actually use these constants.

Quoted 55 lines

dtc.h +5 -0

+
 struct property {
+	int undefined;  /* if the property is set to undefined, this feild is
+	                   set to PROP_UNDEFINED */
 	char *name;
 	struct data val;
 

@@ -170,6 +175,7 @@ void add_label(struct label **labels, char *label);
 struct property *build_property(char *name, struct data val);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
+void undefine_property(struct property *prop);
 
 struct node *build_node(struct property *proplist, struct node *children);
 struct node *name_node(struct node *node, char *name);

@@ -177,6 +183,7 @@ struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
 
 void add_property(struct node *node, struct property *prop);
+void remove_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
 void remove_child(struct node *parent, struct node *child);
 

flattree.c +3 -0

index ead0332..00439e9 100644
@@ -275,6 +275,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 	for_each_property(tree, prop) {
 		int nameoff;
 
+		if (prop->undefined)
+			continue;
+
 		if (streq(prop->name, "name"))
 			seen_name_prop = 1;
 

livetree.c +5 -0

index bf8796b..2ef734d 100644
@@ -51,6 +51,11 @@ struct property *build_property(char *name, struct data val)
 	return new;
 }
 
+void undefine_property(struct property *prop)
+{
+	prop->undefined = 1;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);

@@ -121,11 +126,15 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 		/* Look for a collision, set new value if there is */

livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 131

Quoted 10 lines

livetree.c +2 -5

 		for_each_property(old_node, old_prop) {
 			if (streq(old_prop->name, new_prop->name)) {
-				/* Add new labels to old property */
-				for_each_label(new_prop->labels, l)
-					add_label(&old_prop->labels, l->label);
-
-				old_prop->val = new_prop->val;
+				if (new_prop->undefined) {
+					remove_property(old_node,
 old_prop);

This will lose property labels into the ether as your earlier patch lost node labels.

Quoted 26 lines

livetree.c +7 -0

+				} else {
+					/* Add new labels to old property */
+					for_each_label(new_prop->labels, l)
+						add_label(&old_prop->labels, l->label);
+
+					old_prop->val = new_prop->val;
+				}
 				free(new_prop);
 				new_prop = NULL;

livetree.c +12 -0

 				break;

@@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
 	*p = prop;
 }
 
+void remove_property(struct node *node, struct property *prop)
+{
+	struct property **p;
+	int found = 0;
+
+	p = &node->proplist;
+	while (*p) {
+		if (*p == prop) {
+			*p = (*p)->next;
+			found = 1;
+			break;
+		}

livetree.c void add_property(struct node *node, struct property *prop) · line 215

Quoted 4 lines

livetree.c +4 -0

+		p = &((*p)->next);
+	}
+	/* property not in the node? it's probably an error, so flag it. */
+	assert(found);

assert()s are for finding bugs in code, not detecting errors in user input. AFAICT this *can* be generated by usre input so it should print an error message and if possible continue, not cause a SIGABRT.

Quoted 8 lines

livetree.c +3 -0

+}
+
+
 void add_child(struct node *parent, struct node *child)
 {
 	struct node **p;

Quoted 1 line · David Gibson

On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:

Quoted 16 lines

Quoted 16 lines

livetree.c +12 -0

@@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
 	*p = prop;
 }
 
+void remove_property(struct node *node, struct property *prop)
+{
+	struct property **p;
+	int found = 0;
+
+	p = &node->proplist;
+	while (*p) {
+		if (*p == prop) {
+			*p = (*p)->next;
+			found = 1;
+			break;
+		}

Quoted 8 lines

livetree.c void add_property(struct node *node, struct property *prop) · line 215

Quoted 4 lines

livetree.c +4 -0

+		p = &((*p)->next);
+	}
+	/* property not in the node? it's probably an error, so flag it. */
+	assert(found);

assert()s are for finding bugs in code, not detecting errors in user input. AFAICT this *can* be generated by usre input so it should print an error message and if possible continue, not cause a SIGABRT.

I agree that assert() shouldn't be used if it can be caused by user input, but I don't see how the assert can be triggered by user input. In my mind this is a programming error.

I the only way I see this being triggered by user input is if the user specifies that a property be deleted but it doesn't exist in the current in-memory merged tree. I believe the merge_nodes() routine takes care to avoid problems here by making sure the property already exists in the in-memory merged tree before calling remove_property() on it.