| Message ID | 20240919104238.232704-5-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: dt overlay fixes | expand |
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
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';
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(-)