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

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@addr". In
scenarios where nodes are added with "name@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@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(+)

> On 18-Jan-2023, at 11:14 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> 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@addr". In
> scenarios where nodes are added with "name@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@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.


Hi Dan,

Looking for review comments for this V2 patch set. Please share your feedback

Thanks
Athira
> 
> 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))
> +			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@1") == addr1);
> +	assert(dt_find_by_name(root, "node0_1@2") == addr2);
> +	assert(dt_find_by_name_substr(root, "node@1") == addr1);
> +	assert(dt_find_by_name_substr(root, "node0_1@2") == addr2);
> +	dt_free(root);
> +
> 	return 0;
> }
> 
> diff --git a/include/device.h b/include/device.h
> index 93fb90ff..b6a1a813 100644
> --- a/include/device.h
> +++ b/include/device.h
> @@ -184,6 +184,9 @@ 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 substring */
> +struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name);
> +
> /* 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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

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@addr". In
> scenarios where nodes are added with "name@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@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@1
/node0_1@2

the substring 'node0', 'node0@' and 'node0_@' all matches with 'node@1' device
tree node.
Is this expected ?

Also, what do you expect dt_find_by_name_substr() to return for string
like 'node0' and 'node0_' ? NULL or node '/node0_1@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@1") == addr1);
> +	assert(dt_find_by_name(root, "node0_1@2") == addr2);
> +	assert(dt_find_by_name_substr(root, "node@1") == addr1);
> +	assert(dt_find_by_name_substr(root, "node0_1@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.

Thanks,
-Mahesh.

> On 30-Jan-2023, at 3:14 PM, Mahesh J Salgaonkar <mahesh@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@addr". In
>> scenarios where nodes are added with "name@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@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@1
> /node0_1@2
> 
> the substring 'node0', 'node0@' and 'node0_@' all matches with 'node@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@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@1") == addr1);
>> +	assert(dt_find_by_name(root, "node0_1@2") == addr2);
>> +	assert(dt_find_by_name_substr(root, "node@1") == addr1);
>> +	assert(dt_find_by_name_substr(root, "node0_1@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.