Re: [PATCH v2 04/10] libfdt: overlay: Factor out fixup and fragment path parsing helpers

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

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

On Wed, Oct 28, 2026 at 02:46:53PM -0700, Douglas Anderson wrote:
> In preparation for updating the base device tree's `/__local_fixups__`
> node during `fdt_overlay_apply()`, factor out two helper functions in
> `libfdt/fdt_overlay.c`:
> 
> - Factor out `overlay_parse_fixup_entry()` from
>   `overlay_fixup_phandle()` to parse `"path:name:offset"` entries in
>   `/__fixups__` properties.
> - Factor out `overlay_split_fragment_path()` from
>   `overlay_symbol_update()` to validate and split
>   `"/<fragment>/__overlay__[/<rel>]"` paths into the fragment name and
>   relative subnode path.
> 
> This commit is pure code movement with no functional change.

Conceptually good, but has a couple of warts.

> 
> Assisted-by: Gemini:gemini-pro-prerelease
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - New patch factoring out overlay_parse_fixup_entry() and
>   overlay_split_fragment_path() helpers in libfdt/fdt_overlay.c
> 
>  libfdt/fdt_overlay.c | 226 +++++++++++++++++++++++++++++--------------
>  1 file changed, 154 insertions(+), 72 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 51a3859620a4..f9d1a555ed5d 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -351,6 +351,72 @@ static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
>  						   sizeof(phandle_prop));
>  }
>  
> +/**
> + * overlay_parse_fixup_entry - Parse one "path:name:offset" entry from /__fixups__
> + * @valuep: In/out pointer to the current NUL-terminated fixup string
> + * @lenp: In/out remaining length of the /__fixups__ property value

It would be good to describe how the in/out values are updated.

> + * @pathp: Output pointer to the target node path
> + * @path_lenp: Output length of @pathp in bytes
> + * @namep: Output pointer to the target property name
> + * @name_lenp: Output length of @namep in bytes
> + * @poffsetp: Output byte offset within the target property


[snip]
> +/**
> + * overlay_split_fragment_path - Split "/<frag>/__overlay__[/<rel>]" into parts
> + * @path: Input NUL-terminated path property string
> + * @path_len: Length of @path in bytes (including trailing NUL)
> + * @frag_namep: Output pointer to the fragment name
> + * @frag_name_lenp: Output length of @frag_namep in bytes
> + * @rel_pathp: Output pointer to the relative subnode path after __overlay__
> + * @rel_path_lenp: Output length of @rel_pathp in bytes
> + * @is_overlayp: Set to true if @path is under an __overlay__ subnode

Returning the bool this way is clunky.  Better to use the return
value.

> + * returns:
> + *      0 on success
> + *      -FDT_ERR_BADVALUE if @path is not a valid NUL-terminated '/' path
> + */
> +static int overlay_split_fragment_path(const char *path, int path_len,
> +				       const char **frag_namep,
> +				       int *frag_name_lenp,
> +				       const char **rel_pathp,
> +				       int *rel_path_lenp,
> +				       bool *is_overlayp)
> +{
> +	int len, frag_name_len, rel_path_len;
> +	const char *s, *e;
> +	const char *frag_name;
> +	const char *rel_path;
> +
> +	*is_overlayp = false;
> +
> +	/* verify it's a string property (terminated by a single \0) */
> +	if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1])
> +		return -FDT_ERR_BADVALUE;
> +
> +	/* keep end marker to avoid strlen() */
> +	e = path + path_len;
> +
> +	if (*path != '/')
> +		return -FDT_ERR_BADVALUE;
> +
> +	/* get fragment name first */
> +	s = strchr(path + 1, '/');
> +	if (!s) {
> +		/* Symbol refers to something that won't end
> +		 * up in the target tree */
> +		return 0;
> +	}
> +
> +	frag_name = path + 1;
> +	frag_name_len = s - path - 1;
> +
> +	/* verify format; safe since "s" lies in \0 terminated prop */
> +	len = sizeof("/__overlay__/") - 1;
> +	if ((e - s) > len && (memcmp(s, "/__overlay__/", len) == 0)) {
> +		/* /<fragment-name>/__overlay__/<relative-subnode-path> */
> +		rel_path = s + len;
> +		rel_path_len = e - rel_path - 1;
> +	} else if ((e - s) == len
> +		   && (memcmp(s, "/__overlay__", len - 1) == 0)) {
> +		/* /<fragment-name>/__overlay__ */
> +		rel_path = "";
> +		rel_path_len = 0;
> +	} else {
> +		/* Symbol refers to something that won't end
> +		 * up in the target tree */
> +		return 0;
> +	}
> +
> +	*frag_namep = frag_name;
> +	*frag_name_lenp = frag_name_len;
> +	*rel_pathp = rel_path;
> +	*rel_path_lenp = rel_path_len;
> +	*is_overlayp = true;
> +	return 0;
> +}
> +
>  /**
>   * overlay_symbol_update - Update the symbols of base tree after a merge
>   * @fdt: Base Device Tree blob
> @@ -907,7 +1017,6 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  {
>  	int root_sym, ov_sym, prop, path_len, fragment, target;
>  	int len, frag_name_len, ret, rel_path_len;
> -	const char *s, *e;
>  	const char *path;
>  	const char *name;
>  	const char *frag_name;
> @@ -934,47 +1043,20 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  
>  	/* iterate over each overlay symbol */
>  	fdt_for_each_property_offset(prop, fdto, ov_sym) {
> +		bool is_overlay;
> +
>  		path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
>  		if (!path)
>  			return path_len;
>  
> -		/* verify it's a string property (terminated by a single \0) */
> -		if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1])
> -			return -FDT_ERR_BADVALUE;
> -
> -		/* keep end marker to avoid strlen() */
> -		e = path + path_len;
> -
> -		if (*path != '/')
> -			return -FDT_ERR_BADVALUE;
> -
> -		/* get fragment name first */
> -		s = strchr(path + 1, '/');
> -		if (!s) {
> -			/* Symbol refers to something that won't end
> -			 * up in the target tree */
> -			continue;
> -		}
> -
> -		frag_name = path + 1;
> -		frag_name_len = s - path - 1;
> -
> -		/* verify format; safe since "s" lies in \0 terminated prop */
> -		len = sizeof("/__overlay__/") - 1;
> -		if ((e - s) > len && (memcmp(s, "/__overlay__/", len) == 0)) {
> -			/* /<fragment-name>/__overlay__/<relative-subnode-path> */
> -			rel_path = s + len;
> -			rel_path_len = e - rel_path - 1;
> -		} else if ((e - s) == len
> -			   && (memcmp(s, "/__overlay__", len - 1) == 0)) {
> -			/* /<fragment-name>/__overlay__ */
> -			rel_path = "";
> -			rel_path_len = 0;
> -		} else {
> -			/* Symbol refers to something that won't end
> -			 * up in the target tree */
> +		ret = overlay_split_fragment_path(path, path_len,
> +						  &frag_name, &frag_name_len,
> +						  &rel_path, &rel_path_len,
> +						  &is_overlay);
> +		if (ret)
> +			return ret;
> +		if (!is_overlay)
>  			continue;
> -		}
>  
>  		/* find the fragment index in which the symbol lies */
>  		ret = fdt_subnode_offset_namelen(fdto, 0, frag_name,
> -- 
> 2.56.0.313.g2479322e68-goog
> 
> 

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