[PATCH 0/4] Series short description

[PATCH 0/4] Series short description

[John Bonesio]

The following series implements...

1) new patch to update error messaging
2) new patch to remove existing nodes
3) patch to reference nodes by path or label/path at the root level
   I chose the following syntax:

   &{/path/to/a/node}       [existing syntax, now is allowed at the root level]
   &label                   [existing syntax]
   &{label}                 [new syntax]
   &{label/path/to/subnode} [new syntax]

4) patch to undefine properties

- John

---

John Bonesio (4):
      Create new and use new print_error that uses printf style formatting.
      Implements new features for updating existing device tree nodes.
      Allow nodes at the root to be specified by path as well as by label.
      Create a new property value that means 'undefined'.


 dtc-lexer.l  |   34 ++++++++++++++++++++++++--
 dtc-parser.y |   47 +++++++++++++++++++++++++++---------
 dtc.h        |    8 ++++++
 flattree.c   |    3 ++
 livetree.c   |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 srcpos.c     |   21 ++++++++++------
 srcpos.h     |    2 ++
 7 files changed, 163 insertions(+), 28 deletions(-)

-- 
Signature

[PATCH 1/4] Create new and use new print_error that uses printf style formatting.

[John Bonesio]

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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

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

diff --git a/dtc-parser.y b/dtc-parser.y
index 0aaf8e8..b58ba8e 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -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;
 }
diff --git a/srcpos.c b/srcpos.c
index 87d7f17..2dbc874 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -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);
 }
 
diff --git a/srcpos.h b/srcpos.h
index 985f847..bd7966e 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -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 *, ...)

Re: [PATCH 1/4] Create new and use new print_error that uses printf style formatting.

[Grant Likely]

On Wed, Oct 20, 2010 at 02:44:58PM -0700, John Bonesio wrote:
> 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>

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.

> ---
> 
>  dtc-parser.y |   28 ++++++++++++++++++----------
>  srcpos.c     |   21 +++++++++++++--------
>  srcpos.h     |    2 ++
>  3 files changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 0aaf8e8..b58ba8e 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -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;
>  }
> diff --git a/srcpos.c b/srcpos.c
> index 87d7f17..2dbc874 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -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);
>  }
>  
> diff --git a/srcpos.h b/srcpos.h
> index 985f847..bd7966e 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -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 *, ...)
> 

Re: [PATCH 1/4] Create new and use new print_error that uses printf style formatting.

[Jon Loeliger]

> On Wed, Oct 20, 2010 at 02:44:58PM -0700, John Bonesio wrote:
> > 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> 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.


Thanks, Grant.  I'll pick this patch up now too, and the
subsequent patch 1/4 from the next series about labels refs.

jdl

[PATCH 2/4] Implements new features for updating existing device tree nodes.

[John Bonesio]

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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

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

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 081e13a..80a886a 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -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);
diff --git a/dtc-parser.y b/dtc-parser.y
index b58ba8e..1ba0478 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -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:
diff --git a/dtc.h b/dtc.h
index b36ac5d..a7f3667 100644
--- a/dtc.h
+++ b/dtc.h
@@ -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);
diff --git a/livetree.c b/livetree.c
index 13c5f10..21a4c48 100644
--- a/livetree.c
+++ b/livetree.c
@@ -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));

Re: [PATCH 2/4] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Wed, Oct 20, 2010 at 02:45:06PM -0700, John Bonesio wrote:
> 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Patch looks good, but it needs a test case before it can be merged.
Also, as mentioned earlier, the switch to get_node_by_ref() should be
applied before this patch.

g.

> ---
> 
>  dtc-lexer.l  |    6 ++++++
>  dtc-parser.y |   11 +++++++++++
>  dtc.h        |    1 +
>  livetree.c   |   18 ++++++++++++++++++
>  4 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 081e13a..80a886a 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -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);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index b58ba8e..1ba0478 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -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:
> diff --git a/dtc.h b/dtc.h
> index b36ac5d..a7f3667 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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);
> diff --git a/livetree.c b/livetree.c
> index 13c5f10..21a4c48 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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));
> 

Re: [PATCH 2/4] Implements new features for updating existing device tree nodes.

[David Gibson]

On Wed, Oct 20, 2010 at 02:45:06PM -0700, John Bonesio wrote:
> 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.

[snip]
> +	| 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);
> +		}

This still has the problem of labels disappearing into the ether.

>  	;
>  
>  nodedef:
> diff --git a/dtc.h b/dtc.h
> index b36ac5d..a7f3667 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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);
> diff --git a/livetree.c b/livetree.c
> index 13c5f10..21a4c48 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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));
> 

-- 
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

[PATCH 3/4] Allow nodes at the root to be specified by path as well as by label.

[John Bonesio]

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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

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

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 80a886a..216a3d2 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -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);
diff --git a/dtc-parser.y b/dtc-parser.y
index 1ba0478..0a74c86 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -132,24 +132,22 @@ devicetree:
 		}
 	| devicetree DT_REF nodedef
 		{
-			struct node *target;
+			struct node *target = get_node_by_ref($1, $2);
 
-			target = get_node_by_label($1, $2);
 			if (target)
 				merge_nodes(target, $3);
 			else
-				print_error("label, '%s' not found", $2);
+				print_error("label or path, '%s', not found", $2);
 			$$ = $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);
 		}
 	;
 
diff --git a/livetree.c b/livetree.c
index 21a4c48..bf8796b 100644
--- a/livetree.c
+++ b/livetree.c
@@ -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)

Re: [PATCH 3/4] Allow nodes at the root to be specified by path as well as by label.

[Grant Likely]

On Wed, Oct 20, 2010 at 02:45:13PM -0700, John Bonesio wrote:
> 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>

As already mentioned, but I'm reposting here for the list archive
record, this syntax conflicts with existing convention for referencing
nodes by names defined in the /aliases node.

One more comment below.

> ---
> 
>  dtc-lexer.l  |   26 ++++++++++++++++++++++----
>  dtc-parser.y |   10 ++++------
>  livetree.c   |   20 +++++++++++++++++++-
>  3 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 80a886a..216a3d2 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -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);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 1ba0478..0a74c86 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -132,24 +132,22 @@ devicetree:
>  		}
>  	| devicetree DT_REF nodedef
>  		{
> -			struct node *target;
> +			struct node *target = get_node_by_ref($1, $2);
>  
> -			target = get_node_by_label($1, $2);
>  			if (target)
>  				merge_nodes(target, $3);
>  			else
> -				print_error("label, '%s' not found", $2);
> +				print_error("label or path, '%s', not found", $2);

For clarity, the above hunk should be in a separate patch applied
between patches #1 and #2.  That will avoid having one patch introduce
the DT_REMOVENODE hunk below, and then immediately modify it again in
this patch.  The switch to get_node_by_ref() is useful even without
the label+path syntax, and it should also include a matching testcase.

>  			$$ = $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);
>  		}
>  	;
>  
> diff --git a/livetree.c b/livetree.c
> index 21a4c48..bf8796b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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)
> 

Re: [PATCH 3/4] Allow nodes at the root to be specified by path as well as by label.

[David Gibson]

On Wed, Oct 20, 2010 at 02:45:13PM -0700, John Bonesio wrote:
> 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.

[snip]
> diff --git a/livetree.c b/livetree.c
> index 21a4c48..bf8796b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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, '/');

So, this line is effectively un-const-ing part of the string that ref
points to.

> +
> +		if (p) {
> +			*p = '\0';

And, then you write to it.  Not good at all.  I'm afraid you're going
to have to use a strndup() here.  Or else modify get_node_by_label()
to ignore text after a /.

> +			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)
> 

-- 
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

[PATCH 4/4] Create a new property value that means 'undefined'.

[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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

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

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 216a3d2..efa89b4 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -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;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
@@ -116,10 +122,10 @@ static int pop_input_file(void);
 		}
 
 
-<*>\&{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;
 		}
 
 <*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */
diff --git a/dtc-parser.y b/dtc-parser.y
index 0a74c86..ac9cfd7 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -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
@@ -178,6 +179,11 @@ propdef:
 		{
 			$$ = build_property($1, empty_data);
 		}
+	| DT_PROPNODENAME '=' DT_UNDEFINED ';'
+		{
+			$$ = build_property($1, empty_data);
+			undefine_property($$)
+		}
 	| DT_LABEL propdef
 		{
 			add_label(&$2->labels, $1);
diff --git a/dtc.h b/dtc.h
index a7f3667..b3fca6e 100644
--- a/dtc.h
+++ b/dtc.h
@@ -130,7 +130,12 @@ struct label {
 	struct label *next;
 };
 
+#define PROP_DEFINED (0)
+#define PROP_UNDEFINED (1)
+
 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);
 
diff --git a/flattree.c b/flattree.c
index ead0332..00439e9 100644
--- a/flattree.c
+++ b/flattree.c
@@ -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;
 
diff --git a/livetree.c b/livetree.c
index bf8796b..2ef734d 100644
--- a/livetree.c
+++ b/livetree.c
@@ -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)
+{
+	struct property **p;
+	int found = 0;
+
+	p = &node->proplist;
+	while (*p) {
+		if (*p == prop) {
+			*p = (*p)->next;
+			found = 1;
+			break;
+		}
+		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;

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[Grant Likely]

On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Implementation looks good.
> ---
> 
>  dtc-lexer.l  |   14 ++++++++++----
>  dtc-parser.y |    6 ++++++
>  dtc.h        |    7 +++++++
>  flattree.c   |    3 +++
>  livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
>  5 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 216a3d2..efa89b4 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -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?

>  <*>{LABEL}:	{
>  			DPRINT("Label: %s\n", yytext);
>  			yylval.labelref = xstrdup(yytext);
> @@ -116,10 +122,10 @@ static int pop_input_file(void);
>  		}
>  
>  
> -<*>\&{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.

>  
>  <*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 0a74c86..ac9cfd7 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -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
> @@ -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.

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

Don't bother with the undefine_property function.  Just do:
			$$->undefined = 1;

> +		}

>  	| DT_LABEL propdef
>  		{
>  			add_label(&$2->labels, $1);
> diff --git a/dtc.h b/dtc.h
> index a7f3667..b3fca6e 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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.  :-)

> +
>  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);
>  
> diff --git a/flattree.c b/flattree.c
> index ead0332..00439e9 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -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;
>  
> diff --git a/livetree.c b/livetree.c
> index bf8796b..2ef734d 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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)
> +{
> +	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.

> +		}
> +		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;
> 

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson]

On Wed, Oct 20, 2010 at 11:20:53PM -0600, Grant Likely wrote:
> On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> > 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> Implementation looks good.
> > ---
> > 
> >  dtc-lexer.l  |   14 ++++++++++----
> >  dtc-parser.y |    6 ++++++
> >  dtc.h        |    7 +++++++
> >  flattree.c   |    3 +++
> >  livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
> >  5 files changed, 59 insertions(+), 9 deletions(-)
> > 
> > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > index 216a3d2..efa89b4 100644
> > --- a/dtc-lexer.l
> > +++ b/dtc-lexer.l
> > @@ -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?

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]
> > @@ -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.

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

-- 
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

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[John Bonesio]

On Thu, 2010-10-21 at 17:19 +1100, David Gibson wrote:
> On Wed, Oct 20, 2010 at 11:20:53PM -0600, Grant Likely wrote:
> > On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> > > 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > 
> > Implementation looks good.
> > > ---
> > > 
> > >  dtc-lexer.l  |   14 ++++++++++----
> > >  dtc-parser.y |    6 ++++++
> > >  dtc.h        |    7 +++++++
> > >  flattree.c   |    3 +++
> > >  livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
> > >  5 files changed, 59 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > > index 216a3d2..efa89b4 100644
> > > --- a/dtc-lexer.l
> > > +++ b/dtc-lexer.l
> > > @@ -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?
> 
> 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.

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.

Most lexers for the 'C' language have only one start condition to kick
things off.


> [snip]
> > > @@ -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.
> 
> I.. yeah.. I'm not sure.  I was leaning towards prop = /undef-prop/;
> before, but you've more-or-less persuaded me here.
> 

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson]

On Thu, Oct 21, 2010 at 08:20:59AM -0700, John Bonesio wrote:
> On Thu, 2010-10-21 at 17:19 +1100, David Gibson wrote:
> > On Wed, Oct 20, 2010 at 11:20:53PM -0600, Grant Likely wrote:
> > > On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
[snip]
> > > 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.
> 
> This is probably what David is saying. Generally you want the lexer to
> be context free - meaning everything gets tokenized the same way
> everywhere.

Part of what I was saying, yes.

> 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.

Yeah, it bothered me when it went in, too.  And I've reworked the way
it was done at least once, because it was confusing me.  But I'm
pretty sure it's necessary.

It's basically all because property and node names are lexically
awkward.  They can and do contain a bunch of characters that would
usually be operators or delimiters in a language that's lexically like
C.  But being able to have those property and node names bare means we
have a syntax that's more readable and concise.  But we want the rest
of the syntax to be lexically like C, so we can use C-ish delimiters
and so forth.  So, start conditions it is.

There's also the BYTESTRING start condition.  That's one's also a
convenience / conciseness feature to allow hex blobs of data to be
entered easily.  It's well localized, so again, a reasonable tradeoff
I think.

I just noticed that the INCLUDE state is no longer used, we should
remove it.  And V1 is a hangover from when we supported both dts-v0
and dts-v1 input, which are lexically different, due to the changed
format for integer literals.  We could probably remove that too,
though it would require a bit more care.

-- 
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

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[John Bonesio]

On Wed, 2010-10-20 at 23:20 -0600, Grant Likely wrote:
> On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> > 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/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> Implementation looks good.
> > ---
> > 
> >  dtc-lexer.l  |   14 ++++++++++----
> >  dtc-parser.y |    6 ++++++
> >  dtc.h        |    7 +++++++
> >  flattree.c   |    3 +++
> >  livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
> >  5 files changed, 59 insertions(+), 9 deletions(-)
> > 
> > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > index 216a3d2..efa89b4 100644
> > --- a/dtc-lexer.l
> > +++ b/dtc-lexer.l
> > @@ -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?
> 
> >  <*>{LABEL}:	{
> >  			DPRINT("Label: %s\n", yytext);
> >  			yylval.labelref = xstrdup(yytext);
> > @@ -116,10 +122,10 @@ static int pop_input_file(void);
> >  		}
> >  
> >  
> > -<*>\&{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.
> 
> >  
> >  <*>"&{"{LABEL}{PATHCHAR}*\} { /* label and/or path refererence with braces */
> > diff --git a/dtc-parser.y b/dtc-parser.y
> > index 0a74c86..ac9cfd7 100644
> > --- a/dtc-parser.y
> > +++ b/dtc-parser.y
> > @@ -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
> > @@ -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.

I'd be ok with /undef-prop/ property;

This seems to fit with our /remove-node/ node; syntax. If we do this, I
think it would be better to use
	/remove-prop/ property;


> > +		{
> > +			$$ = build_property($1, empty_data);
> > +			undefine_property($$)
> 
> Don't bother with the undefine_property function.  Just do:
> 			$$->undefined = 1;
> 
> > +		}
> 
> >  	| DT_LABEL propdef
> >  		{
> >  			add_label(&$2->labels, $1);
> > diff --git a/dtc.h b/dtc.h
> > index a7f3667..b3fca6e 100644
> > --- a/dtc.h
> > +++ b/dtc.h
> > @@ -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.  :-)
> 
> > +
> >  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);
> >  
> > diff --git a/flattree.c b/flattree.c
> > index ead0332..00439e9 100644
> > --- a/flattree.c
> > +++ b/flattree.c
> > @@ -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;
> >  
> > diff --git a/livetree.c b/livetree.c
> > index bf8796b..2ef734d 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -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)
> > +{
> > +	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.
> 
> > +		}
> > +		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;
> > 

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson]

On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> 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]
> diff --git a/dtc.h b/dtc.h
> index a7f3667..b3fca6e 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -130,7 +130,12 @@ struct label {
>  	struct label *next;
>  };
>  
> +#define PROP_DEFINED (0)
> +#define PROP_UNDEFINED (1)

You never actually use these constants.

> +
>  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);
>  
> diff --git a/flattree.c b/flattree.c
> index ead0332..00439e9 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -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;
>  
> diff --git a/livetree.c b/livetree.c
> index bf8796b..2ef734d 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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);

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

> +				} 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)
> +{
> +	struct property **p;
> +	int found = 0;
> +
> +	p = &node->proplist;
> +	while (*p) {
> +		if (*p == prop) {
> +			*p = (*p)->next;
> +			found = 1;
> +			break;
> +		}
> +		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.

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

-- 
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

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[John Bonesio]

On Thu, 2010-10-21 at 17:14 +1100, David Gibson wrote:
> On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> > 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]
> > diff --git a/dtc.h b/dtc.h
> > index a7f3667..b3fca6e 100644
> > --- a/dtc.h
> > +++ b/dtc.h
> > @@ -130,7 +130,12 @@ struct label {
> >  	struct label *next;
> >  };
> >  
> > +#define PROP_DEFINED (0)
> > +#define PROP_UNDEFINED (1)
> 
> You never actually use these constants.
> 
> > +
> >  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);
> >  
> > diff --git a/flattree.c b/flattree.c
> > index ead0332..00439e9 100644
> > --- a/flattree.c
> > +++ b/flattree.c
> > @@ -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;
> >  
> > diff --git a/livetree.c b/livetree.c
> > index bf8796b..2ef734d 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -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);
> 
> This will lose property labels into the ether as your earlier patch
> lost node labels.
> 
> > +				} 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)
> > +{
> > +	struct property **p;
> > +	int found = 0;
> > +
> > +	p = &node->proplist;
> > +	while (*p) {
> > +		if (*p == prop) {
> > +			*p = (*p)->next;
> > +			found = 1;
> > +			break;
> > +		}
> > +		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.

Maybe there's a case I'm not considering.

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

Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson]

On Fri, Oct 22, 2010 at 12:50:58PM -0700, John Bonesio wrote:
> On Thu, 2010-10-21 at 17:14 +1100, David Gibson wrote:
> > On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
[snip]
> > > @@ -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;
> > > +		}
> > > +		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.

Well, it does, but remove_property() is called *from* merge_nodes()
and it's called *before* the new property is added to the old node.
So, unless I'm missing something even more subtle, the user attempting
to remove a property which does not exist will indeed trigger this
assert().  This would make a good testcase...

-- 
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