Even though in most cases device nodes are not present directly under
the root node, it's a perfectly valid configuration (e.g. Qemu virt
machine dtb). At the moment, we don't handle this scenario which leads
to unconditional addition of extra leading '/' in the node full path.
This makes the attempt to add such device overlay to fail.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/common/dt-overlay.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index d18bd12bd38d..63b28889de90 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -325,6 +325,7 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
             int node_name_len;
             unsigned int target_path_len = strlen(target_path);
             unsigned int node_full_name_len;
+            unsigned int extra_len;
 
             node_name = fdt_get_name(fdto, subnode, &node_name_len);
 
@@ -332,10 +333,13 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
                 return node_name_len;
 
             /*
-             * Magic number 2 is for adding '/' and '\0'. This is done to keep
-             * the node_full_path in the correct full node name format.
+             * Extra length is for adding '/' and '\0' unless the target path is
+             * root in which case we don't add the '/' at the beginning. This is
+             * done to keep the node_full_path in the correct full node name
+             * format.
              */
-            node_full_name_len = target_path_len + node_name_len + 2;
+            extra_len = (target_path_len > 1) + 1;
+            node_full_name_len = target_path_len + node_name_len + extra_len;
 
             nodes_full_path[node_num] = xmalloc_bytes(node_full_name_len);
 
@@ -344,9 +348,11 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
 
             memcpy(nodes_full_path[node_num], target_path, target_path_len);
 
-            nodes_full_path[node_num][target_path_len] = '/';
+            /* Target is not root - add separator */
+            if ( target_path_len > 1 )
+                nodes_full_path[node_num][target_path_len++] = '/';
 
-            memcpy(nodes_full_path[node_num] + target_path_len + 1,
+            memcpy(nodes_full_path[node_num] + target_path_len,
                     node_name, node_name_len);
 
             nodes_full_path[node_num][node_full_name_len - 1] = '\0';
-- 
2.37.6

On Thu, 19 Sep 2024, Michal Orzel wrote:
> Even though in most cases device nodes are not present directly under
> the root node, it's a perfectly valid configuration (e.g. Qemu virt
> machine dtb). At the moment, we don't handle this scenario which leads
> to unconditional addition of extra leading '/' in the node full path.
> This makes the attempt to add such device overlay to fail.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  xen/common/dt-overlay.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index d18bd12bd38d..63b28889de90 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -325,6 +325,7 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
>              int node_name_len;
>              unsigned int target_path_len = strlen(target_path);
>              unsigned int node_full_name_len;
> +            unsigned int extra_len;
>  
>              node_name = fdt_get_name(fdto, subnode, &node_name_len);
>  
> @@ -332,10 +333,13 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
>                  return node_name_len;
>  
>              /*
> -             * Magic number 2 is for adding '/' and '\0'. This is done to keep
> -             * the node_full_path in the correct full node name format.
> +             * Extra length is for adding '/' and '\0' unless the target path is
> +             * root in which case we don't add the '/' at the beginning. This is
> +             * done to keep the node_full_path in the correct full node name
> +             * format.
>               */
> -            node_full_name_len = target_path_len + node_name_len + 2;
> +            extra_len = (target_path_len > 1) + 1;

I'd prefer to avoid the implicit bool to int conversion. I think it is
mandated by MISRA R10.1, we have a bool exception but I don't think it
would cover this type of usage. For instance:

extra_len = ((target_path_len > 1) ? 1 : 0)  + 1;

or

extra_len = (target_path_len > 1) ? 2 : 1;


> +            node_full_name_len = target_path_len + node_name_len + extra_len;
>  
>              nodes_full_path[node_num] = xmalloc_bytes(node_full_name_len);
>  
> @@ -344,9 +348,11 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
>  
>              memcpy(nodes_full_path[node_num], target_path, target_path_len);
>  
> -            nodes_full_path[node_num][target_path_len] = '/';
> +            /* Target is not root - add separator */
> +            if ( target_path_len > 1 )
> +                nodes_full_path[node_num][target_path_len++] = '/';
>  
> -            memcpy(nodes_full_path[node_num] + target_path_len + 1,
> +            memcpy(nodes_full_path[node_num] + target_path_len,
>                      node_name, node_name_len);
>  
>              nodes_full_path[node_num][node_full_name_len - 1] = '\0';
> -- 
> 2.37.6
>

On 20/09/2024 05:54, Stefano Stabellini wrote:
> 
> 
> On Thu, 19 Sep 2024, Michal Orzel wrote:
>> Even though in most cases device nodes are not present directly under
>> the root node, it's a perfectly valid configuration (e.g. Qemu virt
>> machine dtb). At the moment, we don't handle this scenario which leads
>> to unconditional addition of extra leading '/' in the node full path.
>> This makes the attempt to add such device overlay to fail.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  xen/common/dt-overlay.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
>> index d18bd12bd38d..63b28889de90 100644
>> --- a/xen/common/dt-overlay.c
>> +++ b/xen/common/dt-overlay.c
>> @@ -325,6 +325,7 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
>>              int node_name_len;
>>              unsigned int target_path_len = strlen(target_path);
>>              unsigned int node_full_name_len;
>> +            unsigned int extra_len;
>>
>>              node_name = fdt_get_name(fdto, subnode, &node_name_len);
>>
>> @@ -332,10 +333,13 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
>>                  return node_name_len;
>>
>>              /*
>> -             * Magic number 2 is for adding '/' and '\0'. This is done to keep
>> -             * the node_full_path in the correct full node name format.
>> +             * Extra length is for adding '/' and '\0' unless the target path is
>> +             * root in which case we don't add the '/' at the beginning. This is
>> +             * done to keep the node_full_path in the correct full node name
>> +             * format.
>>               */
>> -            node_full_name_len = target_path_len + node_name_len + 2;
>> +            extra_len = (target_path_len > 1) + 1;
> 
> I'd prefer to avoid the implicit bool to int conversion. I think it is
> mandated by MISRA R10.1, we have a bool exception but I don't think it
> would cover this type of usage. For instance:
> 
> extra_len = ((target_path_len > 1) ? 1 : 0)  + 1;
> 
> or
> 
> extra_len = (target_path_len > 1) ? 2 : 1;
Ok, will do this version.

~Michal