[PATCH] closes #3889

Joel Sherrill joel at rtems.org
Fri Jul 23 22:15:25 UTC 2021


I concur with Gedare's comments.

I would think your commit message would be similar to the subject of the ticket.
This looks like a decent example:

https://git.rtems.org/rtems/commit/?id=6c23252cdd8ea63d0fe13d9f99b7befbf070fe80

Please update and submit. Pay attention to compiler warnings.

On Fri, Jul 23, 2021 at 12:02 PM Gedare Bloom <gedare at rtems.org> wrote:
>
> Hi Zak,
>
> Please provide a useful first commit line. I think I've mentioned this
> before, but the current guidance is found at
> https://devel.rtems.org/wiki/Developer/Git#GitCommits
>
> Put the ticket closing line within the commit message, usually on its
> own line at the end of your  commit message
>
>
> On Thu, Jul 22, 2021 at 6:29 PM Zacchaeus Leung
> <zakthertemsdev at gmail.com> wrote:
> >
> > ---
> >  cpukit/include/rtems/posix/timer.h |  1 +
> >  cpukit/posix/src/psxtimercreate.c  |  1 +
> >  cpukit/posix/src/timergettime.c    | 61 +++++++++++++++++++-----------
> >  3 files changed, 41 insertions(+), 22 deletions(-)
> >
> > diff --git a/cpukit/include/rtems/posix/timer.h b/cpukit/include/rtems/posix/timer.h
> > index bcbf07a65a..7ae089173a 100644
> > --- a/cpukit/include/rtems/posix/timer.h
> > +++ b/cpukit/include/rtems/posix/timer.h
> > @@ -48,6 +48,7 @@ typedef struct {
> >    uint32_t          ticks;      /* Number of ticks of the initialization */
> >    uint32_t          overrun;    /* Number of expirations of the timer    */
> >    struct timespec   time;       /* Time at which the timer was started   */
> > +  clockid_t         clock_type; /* The type of timer */
> >  } POSIX_Timer_Control;
> >
> >  /**
> > diff --git a/cpukit/posix/src/psxtimercreate.c b/cpukit/posix/src/psxtimercreate.c
> > index a63cf1d100..804c7a41e7 100644
> > --- a/cpukit/posix/src/psxtimercreate.c
> > +++ b/cpukit/posix/src/psxtimercreate.c
> > @@ -91,6 +91,7 @@ int timer_create(
> >    ptimer->timer_data.it_value.tv_nsec    = 0;
> >    ptimer->timer_data.it_interval.tv_sec  = 0;
> >    ptimer->timer_data.it_interval.tv_nsec = 0;
> > +  ptimer->clock_type = clock_id;
> >
> >    _Watchdog_Preinitialize( &ptimer->Timer, _Per_CPU_Get_snapshot() );
> >    _Watchdog_Initialize( &ptimer->Timer, _POSIX_Timer_TSR );
> > diff --git a/cpukit/posix/src/timergettime.c b/cpukit/posix/src/timergettime.c
> > index ee2a566f0e..ff2176ac60 100644
> > --- a/cpukit/posix/src/timergettime.c
> > +++ b/cpukit/posix/src/timergettime.c
> > @@ -28,6 +28,7 @@
> >  #include <rtems/score/todimpl.h>
> >  #include <rtems/score/watchdogimpl.h>
> >  #include <rtems/seterr.h>
> > +#include <rtems/timespec.h>
> >
> >  /*
> >   *          - When a timer is initialized, the value of the time in
> > @@ -35,39 +36,55 @@
> >   *          - When this function is called, it returns the difference
> >   *            between the current time and the initialization time.
> >   */
> > -
> avoid random whitespace changes.
>
> >  int timer_gettime(
> >    timer_t            timerid,
> >    struct itimerspec *value
> > -)
> > +)
> avoid random whitespace changes. I can't even tell what this one changes.
>
> >  {
> >    POSIX_Timer_Control *ptimer;
> > -  ISR_lock_Context     lock_context;
> > -  uint64_t             now;
> > -  uint32_t             remaining;
> > +  ISR_lock_Context lock_context;
> > +  uint32_t remaining;
> > +  Per_CPU_Control *cpu;
> > +  struct timespec *now;
> > +  struct timespec *expire;
> > +  struct timespec *result;
> >
> > -  if ( !value )
> > -    rtems_set_errno_and_return_minus_one( EINVAL );
> > +  if (!value)
> > +    rtems_set_errno_and_return_minus_one(EINVAL);
> avoid whitespace changes. This change is inconsistent with our coding
> conventions.
>
> >
> > -  ptimer = _POSIX_Timer_Get( timerid, &lock_context );
> > -  if ( ptimer != NULL ) {
> > -    Per_CPU_Control *cpu;
> > +  ptimer = _POSIX_Timer_Get(timerid, &lock_context);
> avoid whitespace changes. This change is inconsistent with our coding
> conventions.
>
> > +  if (ptimer == NULL) {
> add spaces after ( and before ), e.g.,
> if ( ptimer == NULL ) {
>     ^                        ^
>
> > +    rtems_set_errno_and_return_minus_one(EINVAL);
> Add spaces around EINVAL
>
> > +  }
> >
> > -    cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
> > -    now = cpu->Watchdog.ticks;
> > +  cpu = _POSIX_Timer_Acquire_critical(ptimer, &lock_context);
> avoid whitespace changes. This change is inconsistent with our coding
> conventions.
>
> > +  rtems_timespec_from_ticks(ptimer->Timer.expire, expire);
> This isn't correct. expire is an uninitialized pointer.  How do you
> compile and run this code? I'm surprised it passes tests with this
> kind of bug.
>
> What you should do instead is declare your struct timespec variable
> and pass a pointer to it, e.g.,
> struct timespec expire;   <-- put that above
> rtems_timespec_from_ticks( ptimer->Timer.expire, &expire );
>                                              ^
>           ^          ^
> don't forget to add spaces after ( and before ). Use & to pass the
> pointer to the local stack-allocated expire struct.
>
> >
> > -    if ( now < ptimer->Timer.expire ) {
> > -      remaining = (uint32_t) ( ptimer->Timer.expire - now );
> > +  if (ptimer->clock_type == CLOCK_MONOTONIC) {
>            ^
>             ^
> add spaces after ( and before )
>
> > +  _Timecounter_Nanouptime(now);
> This isn't correct. now is an uninitialized pointer.  How do you
> compile and run this code? I'm surprised it passes tests with this
> kind of bug.
>
> Declare now like expire, and pass a pointer to the stack-local variable.
>
> > +  }
> > +  if (ptimer-
> >clock_type == CLOCK_REALTIME) {
> add spaces after ( and before )
>
> > +    _TOD_Get(now);
> This isn't correct. now is an uninitialized pointer.  How do you
> compile and run this code? I'm surprised it passes tests with this
> kind of bug.
>
> > +  }
> > +
> > +
> > + if( rtems_timespec_less_than( now, expire )) {
>         ^                                                                  ^
> add space after if and before ) at the end.
>
> > +      rtems_timespec_subtract(now, expire, result);
> This isn't correct. now, expire, and result are uninitialized pointers.
>
> Declare result like now and expire, and pass a reference to the
> stack-local variable.
>
> >      } else {
> > -      remaining = 0;
> > +      result->tv_nsec=0;
> > +      result->tv_sec=0;
> This isn't correct. result is an uninitialized pointer.
>
> Add spaces around =.
>
> >      }
> > +
> > +  (*value).it_value=*result;
> why using *. instead of -> notation?
>
> > +  value->it_interval = ptimer->timer_data.it_interval;
> > +
> > +  _POSIX_Timer_Release(cpu, &lock_context);
> > +  return 0;
> > +}
> > +
> > +
> > +
> > +
> Don't add extra whitespaces. We don't use more than one blank line in
> a row in RTEMS C Code.
>
> >
> > -    _Timespec_From_ticks( remaining, &value->it_value );
> > -    value->it_interval = ptimer->timer_data.it_interval;
> >
> > -    _POSIX_Timer_Release( cpu, &lock_context );
> > -    return 0;
> > -  }
> >
> > -  rtems_set_errno_and_return_minus_one( EINVAL );
> > -}
> > --
> > 2.32.0
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel


More information about the devel mailing list