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

Add a function dt_find_by_name_before_addr() that returns the child node if
it matches till first occurrence 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_before_addr() function
and testcase for the same in core/test/run-device.c

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
Changelog:
v5 -> v6:
- Addressed review comment from Reza. Instead of using new
  variable for "node", use the node "name" as-is since the
  utility is to check the name before addr. Updated the
  test/run-device.c accordingly

v4 -> v5:
- Addressed review comment from Reza and renamed
  dt_find_by_name_substr to dt_find_by_name_before_addr

v3 -> v4:
- Addressed review comment from Mahesh and added his Reviewed-by.

v2 -> v3:
- After review comments from Mahesh, fixed the code
  to consider string upto "@" for both input node name
  as well as child node name. V2 version was comparing
  input node name and child node name upto string length
  of child name. But this will return wrong node if input
  name is larger than child name. Because it will match
  as substring for child name.
  https://lists.ozlabs.org/pipermail/skiboot/2023-January/018596.html

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          | 25 +++++++++++++++++++++++++
 core/test/run-device.c | 14 ++++++++++++++
 include/device.h       |  3 +++
 3 files changed, 42 insertions(+)

Hi Athira,

On Thu, Sep 14, 2023 at 10:02:04PM +0530, Athira Rajeev wrote: 
>+struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
>+{
>+	struct dt_node *child, *match;
>+	char *child_node = NULL;
>+
>+	list_for_each(&root->children, child, list) {
>+		child_node = strdup(child->name);
>+		if (!child_node)
>+			goto err;
>+		child_node = strtok(child_node, "@");
>+		if (!strcmp(child_node, name)) {
>+			free(child_node);
>+			return child;
>+		}
>+
>+		match = dt_find_by_name_before_addr(child, name);
>+		if (match)
>+			return match;

When the function returns on this line, child_node is not freed.

>+	}
>+
>+	free(child_node);
>+err:
>+	return NULL;
>+}

I took at stab at moving free(child_node) inside the loop, and ended up 
with this:

struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
{
	struct dt_node *child, *match = NULL;
	char *child_name = NULL;

	list_for_each(&root->children, child, list) {
		child_name = strdup(child->name);
		if (!child_name)
			return NULL;

		child_name = strtok(child_name, "@");
		if (!strcmp(child_name, name))
			match = child;
		else
			match = dt_find_by_name_before_addr(child, name);

		free(child_name);
		if (match)
			return match;
	}

	return NULL;
}

Does this seem okay to you? If you agree, no need to send another 
revision, I can just fixup during commit. Let me know.

>diff --git a/core/test/run-device.c b/core/test/run-device.c
>index 4a12382bb..fb7a7d2c0 100644
>--- a/core/test/run-device.c
>+++ b/core/test/run-device.c
>@@ -466,6 +466,20 @@ 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_before_addr */
>+	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_before_addr(root, "node") == addr1);
>+	assert(dt_find_by_name_before_addr(root, "node0_") == NULL);

This line appears twice. As above, can fix during commit, so no need for 
a new patch.

>+	assert(dt_find_by_name_before_addr(root, "node0_1") == addr2);
>+	assert(dt_find_by_name_before_addr(root, "node0") == NULL);
>+	assert(dt_find_by_name_before_addr(root, "node0_") == NULL);
>+	dt_free(root);
>+
> 	return 0;
> }
>

> On 15-Sep-2023, at 8:00 PM, Reza Arbab <arbab@linux.ibm.com> wrote:
> 
> Hi Athira,
> 
> On Thu, Sep 14, 2023 at 10:02:04PM +0530, Athira Rajeev wrote: 
>> +struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
>> +{
>> + struct dt_node *child, *match;
>> + char *child_node = NULL;
>> +
>> + list_for_each(&root->children, child, list) {
>> + child_node = strdup(child->name);
>> + if (!child_node)
>> + goto err;
>> + child_node = strtok(child_node, "@");
>> + if (!strcmp(child_node, name)) {
>> + free(child_node);
>> + return child;
>> + }
>> +
>> + match = dt_find_by_name_before_addr(child, name);
>> + if (match)
>> + return match;
> 
> When the function returns on this line, child_node is not freed.
> 
>> + }
>> +
>> + free(child_node);
>> +err:
>> + return NULL;
>> +}
> 
> I took at stab at moving free(child_node) inside the loop, and ended up with this:
> 
> struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
> {
> struct dt_node *child, *match = NULL;
> char *child_name = NULL;
> 
> list_for_each(&root->children, child, list) {
> child_name = strdup(child->name);
> if (!child_name)
> return NULL;
> 
> child_name = strtok(child_name, "@");
> if (!strcmp(child_name, name))
> match = child;
> else
> match = dt_find_by_name_before_addr(child, name);
> 
> free(child_name);
> if (match)
> return match;
> }
> 
> return NULL;
> }
> 
> Does this seem okay to you? If you agree, no need to send another revision, I can just fixup during commit. Let me know.

Hi Reza,

Sure, Change looks good. Thanks for the change and fixup.

Thanks
Athira
> 
>> diff --git a/core/test/run-device.c b/core/test/run-device.c
>> index 4a12382bb..fb7a7d2c0 100644
>> --- a/core/test/run-device.c
>> +++ b/core/test/run-device.c
>> @@ -466,6 +466,20 @@ 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_before_addr */
>> + 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_before_addr(root, "node") == addr1);
>> + assert(dt_find_by_name_before_addr(root, "node0_") == NULL);
> 
> This line appears twice. As above, can fix during commit, so no need for a new patch.
> 
>> + assert(dt_find_by_name_before_addr(root, "node0_1") == addr2);
>> + assert(dt_find_by_name_before_addr(root, "node0") == NULL);
>> + assert(dt_find_by_name_before_addr(root, "node0_") == NULL);
>> + dt_free(root);
>> +
>> return 0;
>> }
>> 
> 
> -- 
> Reza Arbab

On Thu, Sep 14, 2023 at 10:02:04PM +0530, Athira Rajeev wrote:
>Add a function dt_find_by_name_before_addr() that returns the child node if
>it matches till first occurrence 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_before_addr() function
>and testcase for the same in core/test/run-device.c

Series applied to skiboot master with the fixup we discussed.

> On 18-Sep-2023, at 7:42 PM, Reza Arbab <arbab@linux.ibm.com> wrote:
> 
> On Thu, Sep 14, 2023 at 10:02:04PM +0530, Athira Rajeev wrote:
>> Add a function dt_find_by_name_before_addr() that returns the child node if
>> it matches till first occurrence 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_before_addr() function
>> and testcase for the same in core/test/run-device.c
> 
> Series applied to skiboot master with the fixup we discussed.
> 
> -- 
> Reza Arbab

Thanks Reza for picking up the patchset

Athira