Refactor the code to improve readability and address violations of
MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
not contain any expression which has potential side effect").

No functional change.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 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..4342e573c5 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.34.1

On 10.09.2024 21:06, 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:                                         \

Why the re-indentation? The earlier way was pretty intentional, to match
what a non-macroized case label would look like in this switch.

> +        {                                                               \
> +            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;                                           \

Same question as for the earlier patch - where's the potential side
effect?

Jan

On Wed, 11 Sep 2024, Jan Beulich wrote:
> On 10.09.2024 21:06, 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:                                         \
> 
> Why the re-indentation? The earlier way was pretty intentional, to match
> what a non-macroized case label would look like in this switch.
> 
> > +        {                                                               \
> > +            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;                                           \
> 
> Same question as for the earlier patch - where's the potential side
> effect?

Leaving aside the re-indentation / readability changes, I think the fix
is to move the call to guest_handle_cast out of guest_handle_okay.

Since guest_handle_okay is implemented by calling sizeof on *h.p, I am
guessing that passing guest_handle_cast() as h causes problems. I am not
sure if there is a side effect, I cannot spot one, but I can see that
the nested macros could cause issues to a static analyzer.