| Message ID | cfd2873eb69707ec3f33ea888951ae5676c43bab.1727690180.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: address violations of MISRA C Rule 13.6 | expand |
On 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
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
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(-)