| Message ID | 7fbb80bf62fc2e5f91a89375134622366c0b3891.1712305581.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C Rule 16.2 | expand |
On Fri, 5 Apr 2024, Nicola Vetrini wrote: > Refactor the switch so that a violation of > MISRA C Rule 16.2 is resolved (A switch label shall only be used > when the most closely-enclosing compound statement is the body of > a switch statement). > Note that the switch clause ending with the pseudo > keyword "fallthrough" is an allowed exception to Rule 16.3. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> I had to read this a few times, but it is correct Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > The second switch is not the best in terms of readability, so it may > be best to deviate this particular instance. > --- > xen/include/xsm/dummy.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 88039fdd227c..84d3a8ed1c94 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -83,17 +83,17 @@ static always_inline int xsm_default_action( > return 0; > case XSM_TARGET: > if ( evaluate_nospec(src == target) ) > - { > return 0; > + fallthrough; > case XSM_XS_PRIV: > - if ( evaluate_nospec(is_xenstore_domain(src)) ) > - return 0; > - } > - /* fall through */ > + if ( (action == XSM_XS_PRIV) && > + evaluate_nospec(is_xenstore_domain(src)) ) > + return 0; > + fallthrough; > case XSM_DM_PRIV: > if ( target && evaluate_nospec(src->target == target) ) > return 0; > - /* fall through */ > + fallthrough; > case XSM_PRIV: > if ( is_control_domain(src) ) > return 0; > -- > 2.34.1 >
On 4/5/24 05:14, Nicola Vetrini wrote: > Refactor the switch so that a violation of > MISRA C Rule 16.2 is resolved (A switch label shall only be used > when the most closely-enclosing compound statement is the body of > a switch statement). > Note that the switch clause ending with the pseudo > keyword "fallthrough" is an allowed exception to Rule 16.3. > To give you a quick response, on cursory review, I do not believe the two are equivalent. I have never been a fan of the Duff's device in use here, as it makes reasoning about all the variations difficult. I unrolled all of this once before, and I recall it being a bit more intricate than this. v/r, dps
On 2024-04-09 16:02, Daniel P. Smith wrote: > On 4/5/24 05:14, Nicola Vetrini wrote: >> Refactor the switch so that a violation of >> MISRA C Rule 16.2 is resolved (A switch label shall only be used >> when the most closely-enclosing compound statement is the body of >> a switch statement). >> Note that the switch clause ending with the pseudo >> keyword "fallthrough" is an allowed exception to Rule 16.3. >> > > To give you a quick response, on cursory review, I do not believe the > two are equivalent. I have never been a fan of the Duff's device in use > here, as it makes reasoning about all the variations difficult. I > unrolled all of this once before, and I recall it being a bit more > intricate than this. > > v/r, > dps Hi, basically what I was trying to accomplish is this: XSM_TARGET will either enter the if and return, or fallthrough to XSM_XS_PRIV, but then it won't enter the if (as it did before because in order to reach this second case's if statement it had to also satisfy the first condition (evaluate_nospec(src == target)), which would then cause the XSM_TARGET phase to return 0). And then if either the action != XSM_XS_PRIV or the other condition is not satisfied we go down to XSM_DM_PRIV, just like before. I may have made a logical mistake somewhere, of course, but it seems the same thing semantically, by relying on the fact that case XSM_TARGET: if (...) { return 0; case XSM_XS_PRIV: ... } can be rewritten as case XSM_TARGET: if (...) { return 0; } fallthrough; case XSM_XS_PRIV: if (action == XSM_XS_PRIV && ...) { ... } case XSM_DM_PRIV: because there wasn't other code after the closed bracket of the if in the XSM_XS_PRIV case (only a fallthough that remained as is).
On 4/9/24 16:00, Nicola Vetrini wrote: > On 2024-04-09 16:02, Daniel P. Smith wrote: >> On 4/5/24 05:14, Nicola Vetrini wrote: >>> Refactor the switch so that a violation of >>> MISRA C Rule 16.2 is resolved (A switch label shall only be used >>> when the most closely-enclosing compound statement is the body of >>> a switch statement). >>> Note that the switch clause ending with the pseudo >>> keyword "fallthrough" is an allowed exception to Rule 16.3. >>> >> >> To give you a quick response, on cursory review, I do not believe the >> two are equivalent. I have never been a fan of the Duff's device in >> use here, as it makes reasoning about all the variations difficult. I >> unrolled all of this once before, and I recall it being a bit more >> intricate than this. >> >> v/r, >> dps > > Hi, > > basically what I was trying to accomplish is this: XSM_TARGET will > either enter the if and return, or fallthrough to XSM_XS_PRIV, but then > it won't enter the if (as it did before because in order to reach this > second case's if statement it had to also satisfy the first condition > (evaluate_nospec(src == target)), which would then cause the XSM_TARGET > phase to return 0). And then if either the action != XSM_XS_PRIV or the > other condition is not satisfied we go down to XSM_DM_PRIV, just like > before. I may have made a logical mistake somewhere, of course, but it > seems the same thing semantically, by relying on the fact that > > case XSM_TARGET: > if (...) { > return 0; > case XSM_XS_PRIV: > ... > } > > can be rewritten as > > case XSM_TARGET: > if (...) { return 0; } > fallthrough; > case XSM_XS_PRIV: > if (action == XSM_XS_PRIV && ...) { ... } > case XSM_DM_PRIV: > > because there wasn't other code after the closed bracket of the if in > the XSM_XS_PRIV case (only a fallthough that remained as is). Yes, upon a closer look, you are correct. I just did a quick read earlier since this has been sitting for a few days. I missed that you did in fact have it falling all the way down through all the subsequent checks for each case "entry point" into the privilege hierarchy. Honestly, I probably should look into a comment to document the hierarchy of the checks that is buried in there. Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 88039fdd227c..84d3a8ed1c94 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -83,17 +83,17 @@ static always_inline int xsm_default_action( return 0; case XSM_TARGET: if ( evaluate_nospec(src == target) ) - { return 0; + fallthrough; case XSM_XS_PRIV: - if ( evaluate_nospec(is_xenstore_domain(src)) ) - return 0; - } - /* fall through */ + if ( (action == XSM_XS_PRIV) && + evaluate_nospec(is_xenstore_domain(src)) ) + return 0; + fallthrough; case XSM_DM_PRIV: if ( target && evaluate_nospec(src->target == target) ) return 0; - /* fall through */ + fallthrough; case XSM_PRIV: if ( is_control_domain(src) ) return 0;
Refactor the switch so that a violation of MISRA C Rule 16.2 is resolved (A switch label shall only be used when the most closely-enclosing compound statement is the body of a switch statement). Note that the switch clause ending with the pseudo keyword "fallthrough" is an allowed exception to Rule 16.3. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- The second switch is not the best in terms of readability, so it may be best to deviate this particular instance. --- xen/include/xsm/dummy.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)