1. Patchew
  2. Xen
  3. View series

[XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4

Federico Serafini posted 1 patch 1 month, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/3ec419e30227a8016c28e04524cd36a549aaddcf.1709898466.git.federico.serafini@bugseng.com
xen/common/event_channel.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
[XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Federico Serafini 1 month, 3 weeks ago 
Add missing break statements to address violations of MISRA C:2012
Rule 16.3 ("An unconditional `break' statement shall terminate every
switch-clause").

Add missing default cases to address violations of MISRA C:2012
Rule 16.4 (Every `switch' statement shall have a `default' label).

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/common/event_channel.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 15aec5dcbb..cf19020e49 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
 
     case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
         return arch_virq_is_global(virq);
+
+    default:
+        ASSERT(virq < NR_VIRQS);
+        break;
     }
 
-    ASSERT(virq < NR_VIRQS);
     return true;
 }
 
@@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
         break;
     default:
         ret = -EINVAL;
+        break;
     }
 
 out:
@@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
         case ECS_VIRQ:
             printk(" v=%d", chn->u.virq);
             break;
+        default:
+            /* Nothing to do in other cases. */
+            break;
         }
 
         ssid = xsm_show_security_evtchn(d, chn);
-- 
2.34.1
Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Jan Beulich 1 month, 3 weeks ago 
On 08.03.2024 12:51, Federico Serafini wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>  
>      case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>          return arch_virq_is_global(virq);
> +
> +    default:
> +        ASSERT(virq < NR_VIRQS);
> +        break;
>      }
>  
> -    ASSERT(virq < NR_VIRQS);
>      return true;
>  }

Just for my understanding: The ASSERT() is moved so the "default" would
consist of more than just "break". Why is it that then the "return" isn't
moved, too?

> @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>          break;
>      default:
>          ret = -EINVAL;
> +        break;
>      }

I certainly agree here.

> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>          case ECS_VIRQ:
>              printk(" v=%d", chn->u.virq);
>              break;
> +        default:
> +            /* Nothing to do in other cases. */
> +            break;
>          }

Yes this, just to mention it, while in line with what Misra demands is
pretty meaningless imo: The absence of "default" says exactly what the
comment now says. FTAOD - this is a comment towards the Misra guideline,
not so much towards the specific change here.

One other remark though, considering the specific function we're in: In
a certifiable environment, will there actually be the capability to
issue debug keys? Shouldn't we have a Kconfig option allowing to remove
all that from a build (and then also from relevant scans)?

Jan
Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Federico Serafini 1 month, 3 weeks ago 
On 11/03/24 08:40, Jan Beulich wrote:
> On 08.03.2024 12:51, Federico Serafini wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>>   
>>       case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>>           return arch_virq_is_global(virq);
>> +
>> +    default:
>> +        ASSERT(virq < NR_VIRQS);
>> +        break;
>>       }
>>   
>> -    ASSERT(virq < NR_VIRQS);
>>       return true;
>>   }
> 
> Just for my understanding: The ASSERT() is moved so the "default" would
> consist of more than just "break". Why is it that then the "return" isn't
> moved, too?

No reason in particular.
If preferred, I can move it too.

> 
>> @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>>           break;
>>       default:
>>           ret = -EINVAL;
>> +        break;
>>       }
> 
> I certainly agree here.
> 
>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>           case ECS_VIRQ:
>>               printk(" v=%d", chn->u.virq);
>>               break;
>> +        default:
>> +            /* Nothing to do in other cases. */
>> +            break;
>>           }
> 
> Yes this, just to mention it, while in line with what Misra demands is
> pretty meaningless imo: The absence of "default" says exactly what the
> comment now says. FTAOD - this is a comment towards the Misra guideline,
> not so much towards the specific change here.

Both you and Stefano reviewed the code and agreed on the fact that doing
nothing for the default case is the right thing and now the code
explicitly says that without letting any doubts.
Furthermore, during the reviews it could happen that you notice a switch
where something needs to be done for the default case.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Jan Beulich 1 month, 3 weeks ago 
On 11.03.2024 10:02, Federico Serafini wrote:
> On 11/03/24 08:40, Jan Beulich wrote:
>> On 08.03.2024 12:51, Federico Serafini wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>>>   
>>>       case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>>>           return arch_virq_is_global(virq);
>>> +
>>> +    default:
>>> +        ASSERT(virq < NR_VIRQS);
>>> +        break;
>>>       }
>>>   
>>> -    ASSERT(virq < NR_VIRQS);
>>>       return true;
>>>   }
>>
>> Just for my understanding: The ASSERT() is moved so the "default" would
>> consist of more than just "break". Why is it that then the "return" isn't
>> moved, too?
> 
> No reason in particular.
> If preferred, I can move it too.

I for one would prefer that, yes. But what's needed up front is that we
decide what we want to do _consistently_ in all such cases.

>>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>>           case ECS_VIRQ:
>>>               printk(" v=%d", chn->u.virq);
>>>               break;
>>> +        default:
>>> +            /* Nothing to do in other cases. */
>>> +            break;
>>>           }
>>
>> Yes this, just to mention it, while in line with what Misra demands is
>> pretty meaningless imo: The absence of "default" says exactly what the
>> comment now says. FTAOD - this is a comment towards the Misra guideline,
>> not so much towards the specific change here.
> 
> Both you and Stefano reviewed the code and agreed on the fact that doing
> nothing for the default case is the right thing and now the code
> explicitly says that without letting any doubts.
> Furthermore, during the reviews it could happen that you notice a switch
> where something needs to be done for the default case.

That shouldn't happen during review. Anyone proposing a patch to add such
a comment wants to first have made sure the comment is actually applicable
there. Otherwise we're in "mechanically add comments" territory, which I
think we all agreed we want to avoid.

Jan
Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Federico Serafini 1 month, 3 weeks ago 
On 11/03/24 10:51, Jan Beulich wrote:
> On 11.03.2024 10:02, Federico Serafini wrote:
>> On 11/03/24 08:40, Jan Beulich wrote:
>>> On 08.03.2024 12:51, Federico Serafini wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>>>>    
>>>>        case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>>>>            return arch_virq_is_global(virq);
>>>> +
>>>> +    default:
>>>> +        ASSERT(virq < NR_VIRQS);
>>>> +        break;
>>>>        }
>>>>    
>>>> -    ASSERT(virq < NR_VIRQS);
>>>>        return true;
>>>>    }
>>>
>>> Just for my understanding: The ASSERT() is moved so the "default" would
>>> consist of more than just "break". Why is it that then the "return" isn't
>>> moved, too?
>>
>> No reason in particular.
>> If preferred, I can move it too.
> 
> I for one would prefer that, yes. But what's needed up front is that we
> decide what we want to do _consistently_ in all such cases.

The only reason I left the return at the end is because it is
slightly more compliant to advisory (and not accepted) Rule 15.5 that
says to place a single return at the end of the function and to not use
early returns.

> 
>>>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>>>            case ECS_VIRQ:
>>>>                printk(" v=%d", chn->u.virq);
>>>>                break;
>>>> +        default:
>>>> +            /* Nothing to do in other cases. */
>>>> +            break;
>>>>            }
>>>
>>> Yes this, just to mention it, while in line with what Misra demands is
>>> pretty meaningless imo: The absence of "default" says exactly what the
>>> comment now says. FTAOD - this is a comment towards the Misra guideline,
>>> not so much towards the specific change here.
>>
>> Both you and Stefano reviewed the code and agreed on the fact that doing
>> nothing for the default case is the right thing and now the code
>> explicitly says that without letting any doubts.
>> Furthermore, during the reviews it could happen that you notice a switch
>> where something needs to be done for the default case.
> 
> That shouldn't happen during review. Anyone proposing a patch to add such
> a comment wants to first have made sure the comment is actually applicable
> there. Otherwise we're in "mechanically add comments" territory, which I
> think we all agreed we want to avoid.

What I wanted to say is that adding the comment is not completely
meaningless: it makes the intention of the code explicit and such
intention is double/triple checked.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4
Posted by Stefano Stabellini 1 month, 3 weeks ago 
On Fri, 8 Mar 2024, Federico Serafini wrote:
> Add missing break statements to address violations of MISRA C:2012
> Rule 16.3 ("An unconditional `break' statement shall terminate every
> switch-clause").
> 
> Add missing default cases to address violations of MISRA C:2012
> Rule 16.4 (Every `switch' statement shall have a `default' label).
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.