Short commit message was Re: [rtems commit] rtems: Fix rtems_task_mode()

[Joel Sherrill]

Sorry to pick Sebastian and I know it is too late to fix but in
providing feedback to Ryan and Alexon their commit messages,
"Fix XXX" was a pattern I realized really was not good and
encouraged them to avoid.

Look at https://git.rtems.org/rtems/log/ and see which ones give
you a solid hint and which ones say little beyond "I changed something".

I'm not sure how to turn this into solid guidance for the future
except that the title  message should be short but specific enough
to hint at the purpose of the patch.

I think we all get in a hurry and forget that these messages are
permanent records and there for others (and ourselves) in the future.

No offense.

--joel

On Thu, Mar 18, 2021 at 4:39 AM Sebastian Huber <sebh at rtems.org> wrote:

> Module:    rtems
> Branch:    master
> Commit:    50abce31efeb54efd1957143e3cbf279785d8a9b
> Changeset:
> http://git.rtems.org/rtems/commit/?id=50abce31efeb54efd1957143e3cbf279785d8a9b
>
> Author:    Sebastian Huber <sebastian.huber at embedded-brains.de>
> Date:      Thu Mar 18 10:37:23 2021 +0100
>
> rtems: Fix rtems_task_mode()
>
> Do the ASR and preemption mode change only if requested by the mode
> mask.  The bug was introduced by
> 508f868237225a75e566d9fd304206363cfe441d.
>
> ---
>
>  cpukit/rtems/src/taskmode.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/cpukit/rtems/src/taskmode.c b/cpukit/rtems/src/taskmode.c
> index 377224c..96bed47 100644
> --- a/cpukit/rtems/src/taskmode.c
> +++ b/cpukit/rtems/src/taskmode.c
> @@ -94,31 +94,37 @@ rtems_status_code rtems_task_mode(
>    if ( ( mask & ( RTEMS_ASR_MASK | RTEMS_PREEMPT_MASK ) ) != 0 ) {
>      bool             need_thread_dispatch;
>      ISR_lock_Context lock_context;
> -    bool             previous_asr_is_enabled;
> -    bool             previous_is_preemptible;
>
>      need_thread_dispatch = false;
>
>      _Thread_State_acquire( executing, &lock_context );
>
> -    previous_asr_is_enabled = asr->is_enabled;
> -    asr->is_enabled = !_Modes_Is_asr_disabled( mode_set );
> +    if ( ( mask & RTEMS_ASR_MASK ) != 0 ) {
> +      bool previous_asr_is_enabled;
>
> -    if (
> -      !previous_asr_is_enabled &&
> -        asr->is_enabled &&
> -        asr->signals_pending != 0
> -    ) {
> -      need_thread_dispatch = true;
> -      _Thread_Append_post_switch_action( executing, &api->Signal_action );
> +      previous_asr_is_enabled = asr->is_enabled;
> +      asr->is_enabled = !_Modes_Is_asr_disabled( mode_set );
> +
> +      if (
> +        !previous_asr_is_enabled &&
> +          asr->is_enabled &&
> +          asr->signals_pending != 0
> +      ) {
> +        need_thread_dispatch = true;
> +        _Thread_Append_post_switch_action( executing, &api->Signal_action
> );
> +      }
>      }
>
> -    previous_is_preemptible = executing->is_preemptible;
> -    executing->is_preemptible = _Modes_Is_preempt( mode_set );
> +    if ( ( mask & RTEMS_PREEMPT_MASK ) != 0 ) {
> +      bool previous_is_preemptible;
> +
> +      previous_is_preemptible = executing->is_preemptible;
> +      executing->is_preemptible = _Modes_Is_preempt( mode_set );
>
> -    if ( executing->is_preemptible && !previous_is_preemptible ) {
> -      need_thread_dispatch = true;
> -      _Scheduler_Schedule( executing );
> +      if ( executing->is_preemptible && !previous_is_preemptible ) {
> +        need_thread_dispatch = true;
> +        _Scheduler_Schedule( executing );
> +      }
>      }
>
>      if ( need_thread_dispatch ) {
>
> _______________________________________________
> vc mailing list
> vc at rtems.org
> http://lists.rtems.org/mailman/listinfo/vc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210318/b6528a5e/attachment.html>