Re: [XEN PATCH 4/7] xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Dec 20, 2023 at 6:53 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Federico,
>
> On 20/12/2023 11:03, Federico Serafini wrote:
> > Refactor of the code to have a break statement at the end of the
> > switch-clause. This addresses violations of Rule 16.3
> > ("An unconditional `break' statement shall terminate every
> > switch-clause").
> > No functional change.
> >
> > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > ---
> >   xen/arch/arm/mem_access.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> > index 31db846354..fbcb5471f7 100644
> > --- a/xen/arch/arm/mem_access.c
> > +++ b/xen/arch/arm/mem_access.c
> > @@ -168,10 +168,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, 
> > unsigned long flag,
> >            * If this was a read then it was because of mem_access, but if 
> > it was
> >            * a write then the original get_page_from_gva fault was correct.
> >            */
> > -        if ( flag == GV2M_READ )
> > -            break;
> > -        else
> > +        if ( flag != GV2M_READ )
> >               goto err;
> > +
> > +        break;
>
> On both hunks, I find the original version better as it is easier to
> match with the comment. I also understand that it wouldn't be great to
> add a deviation for this construct. So maybe we should tweak a bit the
> comment?

Simplifying the if-else to a single if is fine by me. Adjusting the
comment to reflect the new logic would help though, so +1 to Julien's
comment.

Thanks,
Tamas