Re: [PATCH 2/3] dtc: Add /./

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5387 bytes --]

On Sat, Mar 01, 2025 at 06:55:03PM +0530, Ayush Singh wrote:
> Allow construction new values for a property using old property values.
> Can be used to append, pre-append, duplicate, etc.

Some examples from your series cover letter would make sense in the
commit message.

This mostly LGTM, a few notes below.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  dtc-lexer.l  |  5 +++++
>  dtc-parser.y |  5 +++++
>  dtc.h        |  1 +
>  livetree.c   | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index de60a70b6bdbcb5ae4336ea4171ad6f645e91b36..5efeca10363e0c9c338b6578be9240c3f42249f0 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -144,6 +144,11 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
>  			return DT_OMIT_NO_REF;
>  		}
>  
> +<*>"/./" {
> +			DPRINT("Keyword: /./\n");
> +			return DT_PREV_PROP;
> +		}
> +
>  <*>{LABEL}:	{
>  			DPRINT("Label: %s\n", yytext);
>  			yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4d5eece526243460203157464e3cd75f781e50e7..c34eb10a1068b5eb6a0f08e5a1db8066217b16bf 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -57,6 +57,7 @@ static bool is_ref_relative(const char *ref)
>  %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>  %token DT_BITS
>  %token DT_DEL_PROP
> +%token DT_PREV_PROP
>  %token DT_DEL_NODE
>  %token DT_OMIT_NO_REF
>  %token <propnodename> DT_PROPNODENAME
> @@ -308,6 +309,10 @@ propdata:
>  		{
>  			$$ = data_merge($1, $2);
>  		}
> +        | propdataprefix DT_PREV_PROP
> +                {
> +                        $$ = data_add_marker($1, PREV_VALUE, NULL);
> +                }
>  	| propdataprefix arrayprefix '>'
>  		{
>  			$$ = data_merge($1, $2.data);
> diff --git a/dtc.h b/dtc.h
> index 86928e1eea9764fe5d74d6dbb987589d65d54b66..175fe3637a31cc28453dc27980f315a171763b49 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -110,6 +110,7 @@ enum markertype {
>  	REF_PHANDLE,
>  	REF_PATH,
>  	LABEL,
> +	PREV_VALUE,
>  	TYPE_UINT8,
>  	TYPE_UINT16,
>  	TYPE_UINT32,
> diff --git a/livetree.c b/livetree.c
> index 93c77d95a320ec05aa355e12920cef9e1c91c26a..89e15c39e9eadb87cf8376fe167a4d9c6002031a 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -139,10 +139,34 @@ struct node *reference_node(struct node *node)
>  	return node;
>  }
>  
> +static struct data data_insert_old_value(struct data d, struct marker *m,
> +					 const struct data *old)

The convention is generall to pass struct data by value, not by pointer.

> +{
> +	unsigned int offset = m->offset;
> +	struct marker *next = m->next;
> +	struct marker *marker = m;

There's no point to this initialisation, because..

> +	struct data new_data;
> +
> +	new_data = data_insert_at_marker(d, marker, old->val, old->len);

.. you could just use 'm' here..

> +
> +	/* Copy all markers from old value */
> +	marker = old->markers;

.. then it is overwritten.

> +	for_each_marker(marker) {
> +		m->next = alloc_marker(marker->offset + offset, marker->type,
> +				       marker->ref);

This will make the new marker have marker->ref aliased with the old
marker.  That's... probably ok... but it makes me nervous.  Might be
better to xstrdup() ref here so each marker has its own copy.

If the marker is a label, and /./ appears multiple times, you'll now
have a duplicate label.  That probably warrants a specific warning, at
least.


> +		m = m->next;
> +	}
> +	m->next = next;
> +
> +	return new_data;
> +}
> +
>  struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  {
>  	struct property *new_prop, *old_prop;
>  	struct node *new_child, *old_child;
> +	bool prev_value_used = false;
> +	struct marker *marker;
>  	struct label *l;
>  
>  	old_node->deleted = 0;
> @@ -172,10 +196,26 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  				for_each_label_withdel(new_prop->labels, l)
>  					add_label(&old_prop->labels, l->label);
>  
> +				marker = new_prop->val.markers;
> +				for_each_marker_of_type(marker, PREV_VALUE) {
> +					new_prop->val = data_insert_old_value(
> +						new_prop->val, marker,
> +						&old_prop->val);
> +					prev_value_used = true;
> +				}
> +
>  				old_prop->val = new_prop->val;
>  				old_prop->deleted = 0;
> -				free(old_prop->srcpos);
> -				old_prop->srcpos = new_prop->srcpos;
> +
> +				if (prev_value_used) {
> +					old_prop->srcpos =
> +						srcpos_extend(old_prop->srcpos,
> +							      new_prop->srcpos);
> +				} else {
> +					free(old_prop->srcpos);
> +					old_prop->srcpos = new_prop->srcpos;
> +				}
> +

Hrm.  I realised there are some pre-existing bugs here: if srcpos has
multiple things chained onto it, we only free the first one.  Likewise
I think we don't properly free the old property value or its markers.

Then again, dtc is a short-lived process, so to an extent we don't
really care about memory leaks.

>  				free(new_prop);
>  				new_prop = NULL;
>  				break;
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]