[Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length

[Athira Rajeev]

> On 09-Jan-2023, at 8:59 PM, Dan Horák <dan at danny.cz> wrote:
> 
> Hi Athira,
> 
> On Thu, 5 Jan 2023 12:41:33 +0530
> Athira Rajeev <atrajeev at linux.vnet.ibm.com> wrote:
> 
>> 
>> 
>>> On 05-Jan-2023, at 12:35 PM, Madhavan Srinivasan <maddy at linux.ibm.com> wrote:
>>> 
>>> 
>>> On Mon,  2 Jan 2023 08:45:22 +0530
>>> Athira Rajeev <atrajeev at linux.vnet.ibm.com> wrote:
>>> 
>>>> Add a function dt_find_by_name_len() that returns the child node if
>>>> it matches the first "n" characters of a given name, otherwise NULL.
>>>> This is helpful for cases with node name like: "name at addr". In
>>>> scenarios where nodes are added with "name at addr" format and if the
>>>> value of "addr" is not known, that node can't be matched with node
>>>> name or addr. Hence matching with substring as node name will return
>>>> the expected result. Patch adds dt_find_by_name_len() function
>>>> and testcase for the same in core/test/run-device.c
>>> 
>>> wouldn't it be better to automatically compare the name up to the "@"
>>> character in the node name when searching for the match instead of
>>> having to hard-code the lengths? I think it should be good enough for
>>> the use case described above.
>>> 
>>> something like
>>> ...
>>> pos = strchr(child->name, '@')
>>> if (!strncmp(child->name, name, pos - child->name))
>>> ...
>>> 
>>> 
>>> 		Dan
>> 
>> Hi Dan,
>> 
>> Thanks for checking the patch.
>> 
>> Comparing upto "@" while searching for the match will restrict the string search only for patterns with "@".
>> By having dt_find_by_name_len which uses length, will be useful for generic substring search for different patterns.
>> So prefered to use length instead of hardcoding character.
>> 
>> Please let us know your thoughts.
> 
> I understand the presented solution is a pretty generic one, but I think
> the question is whether the added complexity brings the benefits
> compared to the simpler "separator char" solution.
> 
> And thinking even more about the generic "length" approach, it might
> bring some false positive hits. Imagine nodes abc at 1, abcd at 2 and you are
> looking for "abc". A search for (abc,3) will match also the "abcd"
> one. And if the search string will always contain the "@" character,
> then specifying the length is not required. And I believe the length
> parameter might be totally redundant, because it can be derived from
> the search string and the new function would be like
> "dt_find_by_name_substr()".
> 
> 
> 	With regards,
> 
> 		Dan


Hi Dan,

Thanks for the response. Makes sense to have the new function as "dt_find_by_name_substr" by comparing upto “@".
I will rework on the changes and post a V2 for this.

Thanks
Athira

> 
>> Thanks
>> Athira
>> 
>>> 
>>>> Signed-off-by: Athira Rajeev <atrajeev at linux.vnet.ibm.com>
>>>> ---
>>>> core/device.c          | 20 ++++++++++++++++++++
>>>> core/test/run-device.c | 11 +++++++++++
>>>> include/device.h       |  4 ++++
>>>> 3 files changed, 35 insertions(+)
>>>> diff --git a/core/device.c b/core/device.c
>>>> index 2de37c74..72c54e85 100644
>>>> --- a/core/device.c
>>>> +++ b/core/device.c
>>>> @@ -395,6 +395,26 @@ struct dt_node *dt_find_by_name(struct dt_node *root, const char *name)
>>>> }
>>>> +struct dt_node *dt_find_by_name_len(struct dt_node *root,
>>>> +					const char *name, int len)
>>>> +{
>>>> +	struct dt_node *child, *match;
>>>> +
>>>> +	if (len <= 0)
>>>> +		return NULL;
>>>> +
>>>> +	list_for_each(&root->children, child, list) {
>>>> +		if (!strncmp(child->name, name, len))
>>>> +			return child;
>>>> +
>>>> +		match = dt_find_by_name_len(child, name, len);
>>>> +		if (match)
>>>> +			return match;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> struct dt_node *dt_new_check(struct dt_node *parent, const char *name)
>>>> {
>>>> 	struct dt_node *node = dt_find_by_name(parent, name);
>>>> diff --git a/core/test/run-device.c b/core/test/run-device.c
>>>> index 4a12382b..8c552103 100644
>>>> --- a/core/test/run-device.c
>>>> +++ b/core/test/run-device.c
>>>> @@ -466,6 +466,17 @@ int main(void)
>>>> 	new_prop_ph = dt_prop_get_u32(ut2, "something");
>>>> 	assert(!(new_prop_ph == ev1_ph));
>>>> 	dt_free(subtree);
>>>> +
>>>> +	/* Test dt_find_by_name_len */
>>>> +	root = dt_new_root("");
>>>> +	addr1 = dt_new_addr(root, "node", 0x1);
>>>> +	addr2 = dt_new_addr(root, "node0_1", 0x2);
>>>> +	assert(dt_find_by_name(root, "node at 1") == addr1);
>>>> +	assert(dt_find_by_name(root, "node0_1 at 2") == addr2);
>>>> +	assert(dt_find_by_name_len(root, "node@", 5) == addr1);
>>>> +	assert(dt_find_by_name_len(root, "node0_1@", 8) == addr2);
>>>> +	dt_free(root);
>>>> +
>>>> 	return 0;
>>>> }
>>>> diff --git a/include/device.h b/include/device.h
>>>> index 93fb90ff..f5e0fb79 100644
>>>> --- a/include/device.h
>>>> +++ b/include/device.h
>>>> @@ -184,6 +184,10 @@ struct dt_node *dt_find_by_path(struct dt_node *root, const char *path);
>>>> /* Find a child node by name */
>>>> struct dt_node *dt_find_by_name(struct dt_node *root, const char *name);
>>>> +/* Find a child node by name and len */
>>>> +struct dt_node *dt_find_by_name_len(struct dt_node *root,
>>>> +                                        const char *name, int len);
>>>> +
>>>> /* Find a node by phandle */
>>>> struct dt_node *dt_find_by_phandle(struct dt_node *root, u32 phandle);
>>>> -- 
>>>> 2.27.0
>>>> _______________________________________________
>>>> Skiboot mailing list
>>>> Skiboot at lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/skiboot
>>> _______________________________________________
>>> Skiboot mailing list
>>> Skiboot at lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/skiboot