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 > `"//__overlay__[/]"` 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 > --- > > 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 "//__overlay__[/]" 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)) { > + /* //__overlay__/ */ > + rel_path = s + len; > + rel_path_len = e - rel_path - 1; > + } else if ((e - s) == len > + && (memcmp(s, "/__overlay__", len - 1) == 0)) { > + /* //__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)) { > - /* //__overlay__/ */ > - rel_path = s + len; > - rel_path_len = e - rel_path - 1; > - } else if ((e - s) == len > - && (memcmp(s, "/__overlay__", len - 1) == 0)) { > - /* //__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