guest_handle_ok()'s expansion contains a sizeof() involving its
first argument guest_handle_cast().
The expansion of the latter, in turn, contains a variable
initialization.
Since MISRA considers the initialization (even of a local variable)
a side effect, the chain of expansions mentioned above violates
MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator shall not
contain any expression which has potential side effect).
Refactor the code to address the rule violation.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- better description;
- preserved original indentation.
---
xen/common/compat/grant_table.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 5ad0debf96..bbb717bf64 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -78,12 +78,15 @@ int compat_grant_table_op(
cmd_op = cmd;
switch ( cmd_op )
{
-#define CASE(name) \
- case GNTTABOP_##name: \
- if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
- gnttab_##name##_compat_t), \
- count)) ) \
- rc = -EFAULT; \
+#define CASE(name) \
+ case GNTTABOP_ ## name: \
+ { \
+ XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h = \
+ guest_handle_cast(uop, gnttab_ ## name ## _compat_t); \
+ \
+ if ( unlikely(!guest_handle_okay(h, count)) ) \
+ rc = -EFAULT; \
+ } \
break
#ifndef CHECK_gnttab_map_grant_ref
--
2.43.0On Mon, 30 Sep 2024, Federico Serafini wrote:
> guest_handle_ok()'s expansion contains a sizeof() involving its
> first argument guest_handle_cast().
> The expansion of the latter, in turn, contains a variable
> initialization.
>
> Since MISRA considers the initialization (even of a local variable)
> a side effect, the chain of expansions mentioned above violates
> MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator shall not
> contain any expression which has potential side effect).
>
> Refactor the code to address the rule violation.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
There is a pending interesting comment from Jan on patch #1 that affects
this patch too, but I think this patch is good even just as a
readability improvement so I'll review it as is
> ---
> Changes in v2:
> - better description;
> - preserved original indentation.
> ---
> xen/common/compat/grant_table.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
> index 5ad0debf96..bbb717bf64 100644
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -78,12 +78,15 @@ int compat_grant_table_op(
> cmd_op = cmd;
> switch ( cmd_op )
> {
> -#define CASE(name) \
> - case GNTTABOP_##name: \
> - if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
> - gnttab_##name##_compat_t), \
> - count)) ) \
> - rc = -EFAULT; \
> +#define CASE(name) \
> + case GNTTABOP_ ## name: \
> + { \
> + XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h = \
> + guest_handle_cast(uop, gnttab_ ## name ## _compat_t); \
> + \
> + if ( unlikely(!guest_handle_okay(h, count)) ) \
> + rc = -EFAULT; \
> + } \
> break
We would typically put the break within the case { }
Other than that, I think this. With that change:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> #ifndef CHECK_gnttab_map_grant_ref
> --
> 2.43.0
>
On 01.10.2024 00:53, Stefano Stabellini wrote:
> On Mon, 30 Sep 2024, Federico Serafini wrote:
>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -78,12 +78,15 @@ int compat_grant_table_op(
>> cmd_op = cmd;
>> switch ( cmd_op )
>> {
>> -#define CASE(name) \
>> - case GNTTABOP_##name: \
>> - if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
>> - gnttab_##name##_compat_t), \
>> - count)) ) \
>> - rc = -EFAULT; \
>> +#define CASE(name) \
>> + case GNTTABOP_ ## name: \
>> + { \
>> + XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h = \
>> + guest_handle_cast(uop, gnttab_ ## name ## _compat_t); \
>> + \
>> + if ( unlikely(!guest_handle_okay(h, count)) ) \
>> + rc = -EFAULT; \
>> + } \
>> break
>
> We would typically put the break within the case { }
That won't work very well with the break not having a semicolon, for the
semicolon to actually be used when invoking the macro. Moving the break
(while adding a semicolon there) as you suggest would then mean the use
site semicolon to end up being an unreachable statement.
Jan
On Tue, 1 Oct 2024, Jan Beulich wrote:
> On 01.10.2024 00:53, Stefano Stabellini wrote:
> > On Mon, 30 Sep 2024, Federico Serafini wrote:
> >> --- a/xen/common/compat/grant_table.c
> >> +++ b/xen/common/compat/grant_table.c
> >> @@ -78,12 +78,15 @@ int compat_grant_table_op(
> >> cmd_op = cmd;
> >> switch ( cmd_op )
> >> {
> >> -#define CASE(name) \
> >> - case GNTTABOP_##name: \
> >> - if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
> >> - gnttab_##name##_compat_t), \
> >> - count)) ) \
> >> - rc = -EFAULT; \
> >> +#define CASE(name) \
> >> + case GNTTABOP_ ## name: \
> >> + { \
> >> + XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h = \
> >> + guest_handle_cast(uop, gnttab_ ## name ## _compat_t); \
> >> + \
> >> + if ( unlikely(!guest_handle_okay(h, count)) ) \
> >> + rc = -EFAULT; \
> >> + } \
> >> break
> >
> > We would typically put the break within the case { }
>
> That won't work very well with the break not having a semicolon, for the
> semicolon to actually be used when invoking the macro. Moving the break
> (while adding a semicolon there) as you suggest would then mean the use
> site semicolon to end up being an unreachable statement.
I didn't think of the extra semicolon posing a problem. In that case, it
is better as it is in this patch
© 2016 - 2024 Red Hat, Inc.