| Message ID | d48b73a3c5c569f95da424fe9e14a7690b1c69f8.1719308599.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violation of MISRA C Rule 13.6 | expand |
On Tue, 25 Jun 2024, Alessandro Zucchelli wrote: > In the file include/xen/event.h macro set_bit is called with argument > current->pause_flags. > Once expanded this set_bit's argument is used in sizeof operations > and thus 'current', being a macro that expands to a function > call with potential side effects, generates a violation. > > To address this violation the value of current is therefore stored in a > variable called 'v' before passing it to macro set_bit. > > No functional change. > > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 25.06.2024 12:14, Alessandro Zucchelli wrote: > --- a/xen/include/xen/event.h > +++ b/xen/include/xen/event.h > @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn) > /* Wait on a Xen-attached event channel. */ > #define wait_on_xen_event_channel(port, condition) \ > do { \ > + struct vcpu *v = current; \ First: As recently indicated elsewhere, any new latching of "current" into a local var should use "curr" or something derived from it (see below), not "v". Second: Macro local variables are at (certain) risk of colliding with outer scope variables. Therefore the common thing we do is add an underscore. Disagreement continues to exist for whether to prefix them or have them be suffixes. An affirmative statement as to Misra's take on underscore-prefixed variables in situations like these would be appreciated. Sadly what the C standard has is somewhat tied to the C library, and hence leaves room for interpretation (and hence for disagreement). Jan
On 01.07.2024 10:16, Jan Beulich wrote: > On 25.06.2024 12:14, Alessandro Zucchelli wrote: >> --- a/xen/include/xen/event.h >> +++ b/xen/include/xen/event.h >> @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn) >> /* Wait on a Xen-attached event channel. */ >> #define wait_on_xen_event_channel(port, condition) \ >> do { \ >> + struct vcpu *v = current; \ > > First: As recently indicated elsewhere, any new latching of "current" into > a local var should use "curr" or something derived from it (see below), not > "v". > > Second: Macro local variables are at (certain) risk of colliding with outer > scope variables. Therefore the common thing we do is add an underscore. > Disagreement continues to exist for whether to prefix them or have them be > suffixes. An affirmative statement as to Misra's take on underscore-prefixed > variables in situations like these would be appreciated. Sadly what the C > standard has is somewhat tied to the C library, and hence leaves room for > interpretation (and hence for disagreement). Why was this patch committed unchanged, considering the comments above? Jan
On Wed, 31 Jul 2024, Jan Beulich wrote: > On 01.07.2024 10:16, Jan Beulich wrote: > > On 25.06.2024 12:14, Alessandro Zucchelli wrote: > >> --- a/xen/include/xen/event.h > >> +++ b/xen/include/xen/event.h > >> @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn) > >> /* Wait on a Xen-attached event channel. */ > >> #define wait_on_xen_event_channel(port, condition) \ > >> do { \ > >> + struct vcpu *v = current; \ > > > > First: As recently indicated elsewhere, any new latching of "current" into > > a local var should use "curr" or something derived from it (see below), not > > "v". > > > > Second: Macro local variables are at (certain) risk of colliding with outer > > scope variables. Therefore the common thing we do is add an underscore. > > Disagreement continues to exist for whether to prefix them or have them be > > suffixes. An affirmative statement as to Misra's take on underscore-prefixed > > variables in situations like these would be appreciated. Sadly what the C > > standard has is somewhat tied to the C library, and hence leaves room for > > interpretation (and hence for disagreement). > > Why was this patch committed unchanged, considering the comments above? Sorry, Jan. I didn't do it on purpose. Due to the code freeze, I added this patch to my for-4.19 queue after acking it. Later, when you provided your review comments, I forgot to remove the patch from my for-4.19 queue. If you want it can be reverted but it is easier if you send a small incremental patch with your suggestion and I'll ack it straight away.
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index f1472ea1eb..48b79f3d62 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn) /* Wait on a Xen-attached event channel. */ #define wait_on_xen_event_channel(port, condition) \ do { \ + struct vcpu *v = current; \ if ( condition ) \ break; \ - set_bit(_VPF_blocked_in_xen, ¤t->pause_flags); \ + set_bit(_VPF_blocked_in_xen, &v->pause_flags); \ smp_mb(); /* set blocked status /then/ re-evaluate condition */ \ if ( condition ) \ { \ - clear_bit(_VPF_blocked_in_xen, ¤t->pause_flags); \ + clear_bit(_VPF_blocked_in_xen, &v->pause_flags); \ break; \ } \ raise_softirq(SCHEDULE_SOFTIRQ); \ @@ -198,7 +199,8 @@ static bool evtchn_usable(const struct evtchn *evtchn) #define prepare_wait_on_xen_event_channel(port) \ do { \ - set_bit(_VPF_blocked_in_xen, ¤t->pause_flags); \ + struct vcpu *v = current; \ + set_bit(_VPF_blocked_in_xen, &v->pause_flags); \ raise_softirq(SCHEDULE_SOFTIRQ); \ smp_mb(); /* set blocked status /then/ caller does his work */ \ } while ( 0 )
In the file include/xen/event.h macro set_bit is called with argument current->pause_flags. Once expanded this set_bit's argument is used in sizeof operations and thus 'current', being a macro that expands to a function call with potential side effects, generates a violation. To address this violation the value of current is therefore stored in a variable called 'v' before passing it to macro set_bit. No functional change. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- xen/include/xen/event.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)