Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2024 19:46, Federico Serafini wrote:
> On 10/09/24 19:41, Federico Serafini wrote:
>> On 10/09/24 16:59, Jan Beulich wrote:
>>> On 10.09.2024 16:57, Jan Beulich wrote:
>>>> On 10.09.2024 12:09, Federico Serafini wrote:
>>>>> Address violations of MISRA C:2012 Rule 16.3:
>>>>> "An unconditional `break' statement shall terminate every
>>>>> switch-clause".
>>>>
>>>> Since in our interpretation "return" is okay too, why is not 
>>>> sufficient to
>>>> simply ...
>>>>
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -170,8 +170,10 @@ static int __init cf_check 
>>>>> parse_phantom_dev(const char *str)
>>>>>       {
>>>>>       case 1: case 2: case 4:
>>>>>           if ( *s )
>>>>> -    default:
>>>>>               return -EINVAL;
>>>>> +        break;
>>>>
>>>> ... insert just this one line here?
>>>
>>> I guess the problem is with the description: It's un-annotated 
>>> fall-through
>>> in this case, not so much the lack of a break (or alike).
>>>
>>> Jan
>>>
>>>>> +    default:
>>>>> +        return -EINVAL;
>>>>>       }
>>>>>       phantom_devs[nr_phantom_devs++] = phantom;
>>>>
>>>
>>
>> Do you prefer this?
>>
>> case 1: case 2: case 4:
>>      if ( *s )
>>          fallthrough;
>>      break;
>> default:
>>      return -EINVAL;
> 
> Oh no, sorry,
> this does not make sense.

First of all I'd prefer if we could leave such untouched. Moving to the
obvious replacement of what's there 

case 1: case 2: case 4:
     if ( *s )
     {
         fallthrough;
default:
         return -EINVAL;
     }
     break;

is arguably not necessarily better than what you were proposing. So I
guess the conclusion is that you're code change is okay(ish) (i.e. if
already we need to touch this), but the description wants changing to
make clear what the problem here is.

Jan