| Message ID | fd3bb0369cc1666a6c4ad79c54ee8772c1e561c2.1725958417.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address violations of MISRA C Rule 16.3 | expand |
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;
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.
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
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;
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(-)