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

[Athira Rajeev]

> On 15-Sep-2023, at 8:00 PM, Reza Arbab <arbab at 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 at 1") == addr1);
>> + assert(dt_find_by_name(root, "node0_1 at 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