Address violations of MISRA C:2012 Rule 16.3:
"An unconditional `break' statement shall terminate every
switch-clause".

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/drivers/passthrough/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 5a446d3dce..a5705def3f 100644
--- 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;
+    default:
+        return -EINVAL;
     }
 
     phantom_devs[nr_phantom_devs++] = phantom;
-- 
2.34.1

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?

Jan

> +    default:
> +        return -EINVAL;
>      }
>  
>      phantom_devs[nr_phantom_devs++] = phantom;

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;
>

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;


-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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