[Skiboot] [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@"

[Athira Rajeev]

> On 30-Jan-2023, at 3:14 PM, Mahesh J Salgaonkar <mahesh at linux.ibm.com> wrote:
> 
> On 2023-01-18 11:14:50 Wed, Athira Rajeev wrote:
>> Add a function dt_find_by_name_substr() that returns the child node if
>> it matches till first occurence at "@" 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_substr() function
>> and testcase for the same in core/test/run-device.c
>> 
>> Signed-off-by: Athira Rajeev <atrajeev at linux.vnet.ibm.com>
>> ---
>> Changelog:
>> v1 -> v2:
>> - Addressed review comment from Dan to update
>>  the utility funtion to search and compare
>>  upto "@". Renamed it as dt_find_by_name_substr.
>> 
>> core/device.c          | 18 ++++++++++++++++++
>> core/test/run-device.c | 11 +++++++++++
>> include/device.h       |  3 +++
>> 3 files changed, 32 insertions(+)
>> 
>> diff --git a/core/device.c b/core/device.c
>> index 2de37c74..df3a5775 100644
>> --- a/core/device.c
>> +++ b/core/device.c
>> @@ -395,6 +395,24 @@ struct dt_node *dt_find_by_name(struct dt_node *root, const char *name)
>> }
>> 
>> 
>> +struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name)
>> +{
>> +	struct dt_node *child, *match;
>> +	char *pos;
>> +
>> +	list_for_each(&root->children, child, list) {
>> +		pos = strchr(child->name, '@');
>> +		if (!strncmp(child->name, name, pos - child->name))
> 
> Shouldn't we care about string length of substring to be checked before
> comparision ? The code assumes that it is always within the limit of
> position of '@' in node name string. Hence, it returns a wrong node
> whose name partially matches with substring passed.
> 
> e.g.
> With following two nodes in deviec tree (as per your test):
> /node at 1
> /node0_1 at 2
> 
> the substring 'node0', 'node0@' and 'node0_@' all matches with 'node at 1' device
> tree node.
> Is this expected ?

Hi Mahesh,

Thanks for reviewing and pointing out.
As you pointed, currently it also returns if name partially matches with substring which is not expected.
I willer-work on changes and post a V3

> 
> Also, what do you expect dt_find_by_name_substr() to return for string
> like 'node0' and 'node0_' ? NULL or node '/node0_1 at 2' ?
> 
>> +			return child;
>> +
>> +		match = dt_find_by_name_substr(child, name);
>> +		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..0e463e58 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_substr */
>> +	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_substr(root, "node at 1") == addr1);
>> +	assert(dt_find_by_name_substr(root, "node0_1 at 2") == addr2);
> 
> Below additional tests are failing:
> 
> 	assert(dt_find_by_name_substr(root, "node0@") == NULL);
> 	assert(dt_find_by_name_substr(root, "node0_@") == NULL);
> 
> Maybe we should add few more test checks for "node0" and "node0_" as well.

Sure, I will fix this in V3

Thanks
Athira 
> 
> Thanks,
> -Mahesh.