| 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 |
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,
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 --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; } /*
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(-)