> On 09-Jan-2023, at 8:59 PM, Dan Horák <d...@danny.cz> wrote:
>
> Hi Athira,
>
> On Thu, 5 Jan 2023 12:41:33 +0530
> Athira Rajeev <atraj...@linux.vnet.ibm.com> wrote:
>
>>
>>
>>> On 05-Jan-2023, at 12:35 PM, Madhavan Srinivasan <ma...@linux.ibm.com>
>>> wrote:
>>>
>>>
>>> On Mon, 2 Jan 2023 08:45:22 +0530
>>> Athira Rajeev <atraj...@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 <atraj...@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
>>>> skib...@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/skiboot
>>> _______________________________________________
>>> Skiboot mailing list
>>> skib...@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/skiboot
