diff mbox series

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

Message ID 20230717032431.33778-1-atrajeev@linux.vnet.ibm.com
State Superseded
Headers show
Series [V5,1/3] core/device: Add function to return child node using name at substring "@" | expand

Commit Message

Athira Rajeev July 17, 2023, 3:24 a.m. UTC 
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:
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          | 31 +++++++++++++++++++++++++++++++
 core/test/run-device.c | 14 ++++++++++++++
 include/device.h       |  3 +++
 3 files changed, 48 insertions(+)

Comments

Reza Arbab Aug. 9, 2023, 9:51 p.m. UTC | #1 
Hi Athira,

I still have a couple of the same questions I asked in v4.

On Mon, Jul 17, 2023 at 08:54:29AM +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.

Given this summary, I don't userstand the following:

>+	assert(dt_find_by_name(root, "node@1") == addr1);
>+	assert(dt_find_by_name(root, "node0_1@2") == addr2);

Is this behavior required? I don't think it makes sense to call this 
function with a second argument containing '@', so I wouldn't expect it 
to match anything in these cases. The function seems to specifically 
enable it:

>+struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
>+{
[snip]
>+	node = strdup(name);
>+	if (!node)
>+		return NULL;
>+	node = strtok(node, "@");

Seems like you could get rid of this and just use name as-is.

I was curious about something else; say we have 'node@1' and 'node@2'.  
Is there an expectation of which it should match?

     addr1 = dt_new_addr(root, "node", 0x1);
     addr2 = dt_new_addr(root, "node", 0x2);
     assert(dt_find_by_name_substr(root, "node") == ???????);
                                                    ^^^^^^^
Athira Rajeev Sept. 11, 2023, 5:15 a.m. UTC | #2 
> On 10-Aug-2023, at 3:21 AM, Reza Arbab <arbab@linux.ibm.com> wrote:
> 
> Hi Athira,
> 
> I still have a couple of the same questions I asked in v4.
> 
> On Mon, Jul 17, 2023 at 08:54:29AM +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.
> 
> Given this summary, I don't userstand the following:
> 
>> + assert(dt_find_by_name(root, "node@1") == addr1);
>> + assert(dt_find_by_name(root, "node0_1@2") == addr2);
> 
> Is this behavior required? I don't think it makes sense to call this function with a second argument containing '@', so I wouldn't expect it to match anything in these cases. The function seems to specifically enable it:

Hi Reza,

Yes makes sense. dt_find_by_name can be removed in this test since its intention is to find device by name.
I will remove these two checks.

> 
>> +struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
>> +{
> [snip]
>> + node = strdup(name);
>> + if (!node)
>> + return NULL;
>> + node = strtok(node, "@");
> 
> Seems like you could get rid of this and just use name as-is.

Ok Reza
> 
> I was curious about something else; say we have 'node@1' and 'node@2'.  Is there an expectation of which it should match?
> 
>    addr1 = dt_new_addr(root, "node", 0x1);
>    addr2 = dt_new_addr(root, "node", 0x2);
>    assert(dt_find_by_name_substr(root, "node") == ???????);
>                                                   ^^^^^^^

In this case, dt_find_by_name_before_addr is not the right function to use.
We have other functions like dt_find_by_name_addr that can be made use of.

I will address other changes in next version

Thanks
Athira
> 
> -- 
> Reza Arbab
diff mbox series

Patch

diff --git a/core/device.c b/core/device.c
index b102dd97..a61a72b0 100644
--- a/core/device.c
+++ b/core/device.c
@@ -395,6 +395,37 @@  struct dt_node *dt_find_by_name(struct dt_node *root, const char *name)
 }
 
 
+struct dt_node *dt_find_by_name_before_addr(struct dt_node *root, const char *name)
+{
+	struct dt_node *child, *match;
+	char *node, *child_node = NULL;
+
+	node = strdup(name);
+	if (!node)
+		return NULL;
+	node = strtok(node, "@");
+	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, node)) {
+			free(child_node);
+			free(node);
+			return child;
+		}
+
+		match = dt_find_by_name_before_addr(child, name);
+		if (match)
+			return match;
+	}
+
+	free(child_node);
+err:
+	free(node);
+	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..f1cb79bf 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@1") == addr1);
+	assert(dt_find_by_name_before_addr(root, "node0_") == NULL);
+	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;
 }
 
diff --git a/include/device.h b/include/device.h
index 93fb90ff..f2402cc4 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_before_addr(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);