[PATCH 05/13] posix: Remove superfluous check

[Gedare Bloom]

On Fri, Feb 19, 2021 at 9:46 AM Joel Sherrill <joel at rtems.org> wrote:
>
>
>
> On Fri, Feb 19, 2021, 10:31 AM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> On Thu, Feb 18, 2021 at 11:56 PM Sebastian Huber
>> <sebastian.huber at embedded-brains.de> wrote:
>> >
>> > On 18/02/2021 20:25, Joel Sherrill wrote:
>> >
>> > >     > -  /*
>> > >     > -   * api may be NULL in case of a thread close in progress
>> > >     > -   */
>> > >     > -  if ( !api )
>> > >     > -    return;
>> > >     > -
>> > >     I believe you, but should we replace this with an assert for now?
>> > >
>> > >
>> > > And a comment explaining why it can't be NULL.
>> >
>> > This is not the only place which uses
>> >
>> > api = executing->API_Extensions[ THREAD_API_POSIX ];
>> >
>> > or
>> >
>> > api = executing->API_Extensions[ THREAD_API_RTEMS ];
>> >
>> > I don't think it makes sense to add a comment to all these places.
>> >
>> > While looking at this, I think we should remove this API_Extensions
>> > indirection and add the structures directly to Thread_Control. This
>> > makes the code more efficient, simpler, faster, and easier to debug.
>> >
>>
>> I think this makes sense from an object-oriented perspective. But,
>> does it (greatly) increase the memory consumption for a system that
>> uses classic tasks but enables posix?
>
>
> Enable POSIX is now misnamed and should have been changed. It only enables some POSIX signals support. But in a review of an executable recently, I realized that posix signal apis inside our teams were built even though they were fundamentally non-functional.
>
"our teams" text-to-speech mangle of "RTEMS"?

Changing the name is an API-level change, but one we can make in
conjunction with the switch from autoconf to waf if you are in favor
of a rename. I'd suggest open a new thread for that discussion.

> Moving this inside may make sense on one level but it breaks the fundamental layering in the design of the super core. The design intent was to not embed knowledge of any particular API in the super core. Thus the apis are viewed as extensions.
>
Agreed. The proper way to do this would be to have an opaque "api
thread object" embedded in the TCB, that gets instantiated by the API
that created/owns the thread, I believe. That's hard to do efficiently
in C though. I think it could be sensible to make an exception, in the
case that the API-specific portion is clearly delineated (e.g., marked
out by #ifdefs) within the supercore.

>
>>
>> > We just have to move a couple of typedefs to score to define these
>> > structures in thread.h.
>> >
>> > #if defined(RTEMS_POSIX_API)
>> > /**
>> >   * This defines the POSIX API support structure associated with
>> >   * each thread in a system with POSIX configured.
>> >   */
>> > typedef struct {
>> >    /**
>> >     * @brief Control block for the sporadic server scheduling policy.
>> >     */
>> >    struct {
>> >      /** The thread of this sporadic control block */
>> >      Thread_Control *thread;
>> >
>> >      /**
>> >       * @brief This is the timer which controls when the thread executes
>> > at high
>> >       * and low priority when using the sporadic server scheduling policy.
>> >       */
>> >      Watchdog_Control Timer;
>> >
>> >      /**
>> >       * @brief The low priority when using the sporadic server scheduling
>> >       * policy.
>> >       */
>> >      Priority_Node Low_priority;
>> >
>> >      /**
>> >       * @brief Replenishment period for sporadic server.
>> >       */
>> >      struct timespec sched_ss_repl_period;
>> >
>> >      /**
>> >       * @brief Initial budget for sporadic server.
>> >       */
>> >      struct timespec sched_ss_init_budget;
>> >
>> >      /**
>> >       * @brief Maximum pending replenishments.
>> >       *
>> >       * Only used by pthread_getschedparam() and pthread_getattr_np().
>> >      */
>> >      int sched_ss_max_repl;
>> >    } Sporadic;
>> >
>> >    /** This is the set of signals which are currently unblocked. */
>> >    sigset_t                signals_unblocked;
>> >    /** This is the set of signals which are currently pending. */
>> >    sigset_t                signals_pending;
>> >
>> >    /**
>> >     * @brief Signal post-switch action in case signals are pending.
>> >     */
>> >    Thread_Action           Signal_action;
>> > } POSIX_API_Control;
>> >
>> > /**
>> >   *  This is the API specific information required by each thread for
>> >   *  the RTEMS API to function correctly.
>> >   *
>> >   */
>> > typedef struct {
>> >    /** This field contains the event control for this task. */
>> >    Event_Control            Event;
>> >    /** This field contains the system event control for this task. */
>> >    Event_Control            System_event;
>> >    /** This field contains the Classic API Signal information for this
>> > task. */
>> >    ASR_Information          Signal;
>> >
>> >    /**
>> >     * @brief Signal post-switch action in case signals are pending.
>> >     */
>> >    Thread_Action            Signal_action;
>> > }  RTEMS_API_Control;
>> >
>> > --
>> > embedded brains GmbH
>> > Herr Sebastian HUBER
>> > Dornierstr. 4
>> > 82178 Puchheim
>> > Germany
>> > email: sebastian.huber at embedded-brains.de
>> > phone: +49-89-18 94 741 - 16
>> > fax:   +49-89-18 94 741 - 08
>> >
>> > Registergericht: Amtsgericht München
>> > Registernummer: HRB 157899
>> > Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
>> > Unsere Datenschutzerklärung finden Sie hier:
>> > https://embedded-brains.de/datenschutzerklaerung/
>> >