All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length
       [not found] ` <246510aa-76f6-f030-d89a-78ac45aef30c@linux.ibm.com>
@ 2023-01-05  7:11   ` Athira Rajeev
  2023-01-09 15:29     ` Dan Horák
  0 siblings, 1 reply; 3+ messages in thread
From: Athira Rajeev @ 2023-01-05  7:11 UTC (permalink / raw)
  To: Dan Horák, skiboot
  Cc: Madhavan Srinivasan, Mahesh J Salgaonkar, Kajol Jain, disgoel,
	linuxppc-dev



> On 05-Jan-2023, at 12:35 PM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
> 
> 
> On Mon,  2 Jan 2023 08:45:22 +0530
> Athira Rajeev <atrajeev@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@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_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.

Thanks
Athira

> 
>> Signed-off-by: Athira Rajeev <atrajeev@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@1") == addr1);
>> +	assert(dt_find_by_name(root, "node0_1@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@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length
  2023-01-05  7:11   ` [Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length Athira Rajeev
@ 2023-01-09 15:29     ` Dan Horák
  2023-01-10  5:28       ` Athira Rajeev
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Horák @ 2023-01-09 15:29 UTC (permalink / raw)
  To: Athira Rajeev
  Cc: Madhavan Srinivasan, Kajol Jain, Mahesh J Salgaonkar, skiboot,
	disgoel, linuxppc-dev

Hi Athira,

On Thu, 5 Jan 2023 12:41:33 +0530
Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:

> 
> 
> > On 05-Jan-2023, at 12:35 PM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
> > 
> > 
> > On Mon,  2 Jan 2023 08:45:22 +0530
> > Athira Rajeev <atrajeev@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@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_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@1, abcd@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

> Thanks
> Athira
> 
> > 
> >> Signed-off-by: Athira Rajeev <atrajeev@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@1") == addr1);
> >> +	assert(dt_find_by_name(root, "node0_1@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@lists.ozlabs.org
> >> https://lists.ozlabs.org/listinfo/skiboot
> > _______________________________________________
> > Skiboot mailing list
> > Skiboot@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/skiboot
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length
  2023-01-09 15:29     ` Dan Horák
@ 2023-01-10  5:28       ` Athira Rajeev
  0 siblings, 0 replies; 3+ messages in thread
From: Athira Rajeev @ 2023-01-10  5:28 UTC (permalink / raw)
  To: Dan Horák
  Cc: Madhavan Srinivasan, Kajol Jain, Mahesh J Salgaonkar, skiboot,
	disgoel, linuxppc-dev



> On 09-Jan-2023, at 8:59 PM, Dan Horák <dan@danny.cz> wrote:
> 
> Hi Athira,
> 
> On Thu, 5 Jan 2023 12:41:33 +0530
> Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
>> 
>> 
>>> On 05-Jan-2023, at 12:35 PM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
>>> 
>>> 
>>> On Mon,  2 Jan 2023 08:45:22 +0530
>>> Athira Rajeev <atrajeev@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@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_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@1, abcd@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@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@1") == addr1);
>>>> +	assert(dt_find_by_name(root, "node0_1@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@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/skiboot
>>> _______________________________________________
>>> Skiboot mailing list
>>> Skiboot@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/skiboot


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-01-10  5:29 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20230102151954.aae52c099e02ef3c4f22fd4e@danny.cz>
     [not found] ` <246510aa-76f6-f030-d89a-78ac45aef30c@linux.ibm.com>
2023-01-05  7:11   ` [Skiboot] [PATCH 1/3] core/device: Add function to return child node using name and length Athira Rajeev
2023-01-09 15:29     ` Dan Horák
2023-01-10  5:28       ` Athira Rajeev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.