diff mbox series

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

Message ID d7a015aaa54fb4722d4970f0f40789efe4d994f9.1703066935.git.federico.serafini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series xen/arm: address violations of MISRA C:2012 Rule 16.3 | expand

Commit Message

Federico Serafini Dec. 20, 2023, 11:03 a.m. UTC
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@bugseng.com>
---
 xen/arch/arm/mem_access.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Julien Grall Dec. 20, 2023, 11:53 a.m. UTC | #1
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@bugseng.com>
> ---
>   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?

Anyway, this code is maintained by Tamas, so I will let him confirm 
which version he prefers.

Cheers,
Tamas K Lengyel Dec. 20, 2023, 3:36 p.m. UTC | #2
On Wed, Dec 20, 2023 at 6:53 AM Julien Grall <julien@xen.org> 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@bugseng.com>
> > ---
> >   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
diff mbox series

Patch

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;
     case XENMEM_access_rx2rw:
     case XENMEM_access_rx:
     case XENMEM_access_r:
@@ -179,10 +179,10 @@  p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * If this was a write then it was because of mem_access, but if it was
          * a read then the original get_page_from_gva fault was correct.
          */
-        if ( flag == GV2M_WRITE )
-            break;
-        else
+        if ( flag != GV2M_WRITE )
             goto err;
+
+        break;
     }
 
     /*