[PATCH] posix: Use cleanup contexts on the stack

Gedare Bloom gedare at rtems.org
Fri Jul 5 15:24:19 UTC 2013


Quick skim looked fine, two questions: is 13 always going to be an
invalid _canceltype, and can you fix the spelling of "cancellation"
everywhere to have two l's instead of one?

On Fri, Jul 5, 2013 at 10:05 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> This change must be in synchronization with a Newlib <pthread.h> change.
> The cleanup contexts are stored on the thread stack.  This is conform to
> the POSIX requirements for the pthread_cleanup_push() and
> pthread_cleanup_pop() statement pair.
>
> Passing an invalid pointer as the routine to pthread_cleanup_push() is
> now a usage error and the behaviour is undefined.
> ---
>  cpukit/posix/include/rtems/posix/cancel.h    |   12 ------
>  cpukit/posix/include/rtems/posix/threadsup.h |    6 ++-
>  cpukit/posix/src/cancelrun.c                 |   26 +++++-------
>  cpukit/posix/src/cleanuppop.c                |   53 +++++---------------------
>  cpukit/posix/src/cleanuppush.c               |   44 +++++++--------------
>  cpukit/posix/src/pthread.c                   |    2 +-
>  testsuites/psxtests/psxcleanup/psxcleanup.c  |    7 ---
>  7 files changed, 40 insertions(+), 110 deletions(-)
>
> diff --git a/cpukit/posix/include/rtems/posix/cancel.h b/cpukit/posix/include/rtems/posix/cancel.h
> index c1fff9c..6ad53d6 100644
> --- a/cpukit/posix/include/rtems/posix/cancel.h
> +++ b/cpukit/posix/include/rtems/posix/cancel.h
> @@ -22,18 +22,6 @@
>  #include <rtems/posix/threadsup.h>
>
>  /**
> - * This structure is used to manage the cancelation handlers.
> - */
> -typedef struct {
> -  /** This field is the Chain Node so we can put these on lists. */
> -  Chain_Node  Node;
> -  /** This field is the cancelation routine. */
> -  void      (*routine)( void * );
> -  /** This field is the argument to the cancelation routine. */
> -  void       *arg;
> -}  POSIX_Cancel_Handler_control;
> -
> -/**
>   * @brief POSIX run thread cancelation.
>   *
>   * This support routine runs through the chain of cancel handlers that
> diff --git a/cpukit/posix/include/rtems/posix/threadsup.h b/cpukit/posix/include/rtems/posix/threadsup.h
> index 80f64dc..c4d75c8 100644
> --- a/cpukit/posix/include/rtems/posix/threadsup.h
> +++ b/cpukit/posix/include/rtems/posix/threadsup.h
> @@ -76,9 +76,11 @@ typedef struct {
>    int                     cancelability_type;
>    /** This indicates if a cancelation has been requested. */
>    int                     cancelation_requested;
> -  /** This is the set of cancelation handlers. */
> -  Chain_Control           Cancellation_Handlers;
>
> +  /**
> +   * @brief LIFO list of cleanup contexts.
> +   */
> +  struct _pthread_cleanup_context *last_cleanup_context;
>  } POSIX_API_Control;
>
>  /**
> diff --git a/cpukit/posix/src/cancelrun.c b/cpukit/posix/src/cancelrun.c
> index 2d73bda..fb3e596 100644
> --- a/cpukit/posix/src/cancelrun.c
> +++ b/cpukit/posix/src/cancelrun.c
> @@ -34,26 +34,22 @@ void _POSIX_Threads_cancel_run(
>    Thread_Control *the_thread
>  )
>  {
> -  POSIX_Cancel_Handler_control      *handler;
> -  Chain_Control                     *handler_stack;
> -  POSIX_API_Control                 *thread_support;
> -  ISR_Level                          level;
> +  struct _pthread_cleanup_context *context;
> +  POSIX_API_Control               *thread_support;
>
> -  thread_support = the_thread->API_Extensions[ THREAD_API_POSIX ];
> -
> -  handler_stack = &thread_support->Cancellation_Handlers;
> +  _Thread_Disable_dispatch();
>
> +  thread_support = the_thread->API_Extensions[ THREAD_API_POSIX ];
>    thread_support->cancelability_state = PTHREAD_CANCEL_DISABLE;
>
> -  while ( !_Chain_Is_empty( handler_stack ) ) {
> -    _ISR_Disable( level );
> -      handler = (POSIX_Cancel_Handler_control *)
> -           _Chain_Tail( handler_stack )->previous;
> -      _Chain_Extract_unprotected( &handler->Node );
> -    _ISR_Enable( level );
> +  context = thread_support->last_cleanup_context;
> +  thread_support->last_cleanup_context = NULL;
> +
> +  _Thread_Enable_dispatch();
>
> -    (*handler->routine)( handler->arg );
> +  while ( context != NULL ) {
> +    ( *context->_routine )( context->_arg );
>
> -    _Workspace_Free( handler );
> +    context = context->_previous;
>    }
>  }
> diff --git a/cpukit/posix/src/cleanuppop.c b/cpukit/posix/src/cleanuppop.c
> index bbadec5..78e2c34 100644
> --- a/cpukit/posix/src/cleanuppop.c
> +++ b/cpukit/posix/src/cleanuppop.c
> @@ -15,67 +15,34 @@
>   */
>
>  #if HAVE_CONFIG_H
> -#include "config.h"
> +  #include "config.h"
>  #endif
>
>  #include <pthread.h>
> -#include <errno.h>
>
>  #include <rtems/system.h>
> -#include <rtems/score/chain.h>
> -#include <rtems/score/isr.h>
>  #include <rtems/score/thread.h>
> -#include <rtems/score/wkspace.h>
> -#include <rtems/posix/cancel.h>
>  #include <rtems/posix/pthread.h>
> -#include <rtems/posix/threadsup.h>
>
>  /*
>   *  18.2.3.1 Establishing Cancellation Handlers, P1003.1c/Draft 10, p. 184
>   */
>
> -void pthread_cleanup_pop(
> -  int    execute
> +void _pthread_cleanup_pop(
> +  struct _pthread_cleanup_context *context,
> +  int                              execute
>  )
>  {
> -  POSIX_Cancel_Handler_control      *handler;
> -  POSIX_Cancel_Handler_control      tmp_handler;
> -  Chain_Control                     *handler_stack;
> -  POSIX_API_Control                 *thread_support;
> -  ISR_Level                          level;
> +  POSIX_API_Control *thread_support;
>
> -  thread_support = _Thread_Executing->API_Extensions[ THREAD_API_POSIX ];
> -
> -  handler_stack = &thread_support->Cancellation_Handlers;
> -
> -  /*
> -   * We need interrupts disabled to safely check the chain and pull
> -   * the last element off.  But we also need dispatching disabled to
> -   * ensure that we do not get prempted and deleted while we are holding
> -   * memory that needs to be freed.
> -   */
> +  if ( execute != 0 ) {
> +    ( *context->_routine )( context->_arg );
> +  }
>
>    _Thread_Disable_dispatch();
> -  _ISR_Disable( level );
> -
> -    if ( _Chain_Is_empty( handler_stack ) ) {
> -      _Thread_Enable_dispatch();
> -      _ISR_Enable( level );
> -      return;
> -    }
> -
> -    handler = (POSIX_Cancel_Handler_control *)
> -        _Chain_Tail( handler_stack )->previous;
> -    _Chain_Extract_unprotected( &handler->Node );
>
> -  _ISR_Enable( level );
> -
> -  tmp_handler = *handler;
> -
> -  _Workspace_Free( handler );
> +  thread_support = _Thread_Executing->API_Extensions[ THREAD_API_POSIX ];
> +  thread_support->last_cleanup_context = context->_previous;
>
>    _Thread_Enable_dispatch();
> -
> -  if ( execute )
> -    (*tmp_handler.routine)( tmp_handler.arg );
>  }
> diff --git a/cpukit/posix/src/cleanuppush.c b/cpukit/posix/src/cleanuppush.c
> index 4fb82c2..52597e6 100644
> --- a/cpukit/posix/src/cleanuppush.c
> +++ b/cpukit/posix/src/cleanuppush.c
> @@ -15,54 +15,38 @@
>   */
>
>  #if HAVE_CONFIG_H
> -#include "config.h"
> +  #include "config.h"
>  #endif
>
>  #include <pthread.h>
> -#include <errno.h>
>
>  #include <rtems/system.h>
> -#include <rtems/score/chain.h>
> -#include <rtems/score/isr.h>
>  #include <rtems/score/thread.h>
> -#include <rtems/score/wkspace.h>
> -#include <rtems/posix/cancel.h>
>  #include <rtems/posix/pthread.h>
> -#include <rtems/posix/threadsup.h>
>
>  /*
>   *  18.2.3.1 Establishing Cancellation Handlers, P1003.1c/Draft 10, p. 184
>   */
>
> -void pthread_cleanup_push(
> -  void   (*routine)( void * ),
> -  void    *arg
> +void _pthread_cleanup_push(
> +  struct _pthread_cleanup_context   *context,
> +  void                            ( *routine )( void * ),
> +  void                              *arg
>  )
>  {
> -  POSIX_Cancel_Handler_control      *handler;
> -  Chain_Control                     *handler_stack;
> -  POSIX_API_Control                 *thread_support;
> +  POSIX_API_Control *thread_support;
>
> -  /*
> -   *  The POSIX standard does not address what to do when the routine
> -   *  is NULL.  It also does not address what happens when we cannot
> -   *  allocate memory or anything else bad happens.
> -   */
> -  if ( !routine )
> -    return;
> +  context->_routine = routine;
> +  context->_arg = arg;
>
> -  _Thread_Disable_dispatch();
> -  handler = _Workspace_Allocate( sizeof( POSIX_Cancel_Handler_control ) );
> -
> -  if ( handler ) {
> -    thread_support = _Thread_Executing->API_Extensions[ THREAD_API_POSIX ];
> +  /* Use an invalid value here */
> +  context->_canceltype = 13;
>
> -    handler_stack = &thread_support->Cancellation_Handlers;
> +  _Thread_Disable_dispatch();
>
> -    handler->routine = routine;
> -    handler->arg = arg;
> +  thread_support = _Thread_Executing->API_Extensions[ THREAD_API_POSIX ];
> +  context->_previous = thread_support->last_cleanup_context;
> +  thread_support->last_cleanup_context = context;
>
> -    _Chain_Append( handler_stack, &handler->Node );
> -  }
>    _Thread_Enable_dispatch();
>  }
> diff --git a/cpukit/posix/src/pthread.c b/cpukit/posix/src/pthread.c
> index f35a000..fc17549 100644
> --- a/cpukit/posix/src/pthread.c
> +++ b/cpukit/posix/src/pthread.c
> @@ -198,7 +198,7 @@ static bool _POSIX_Threads_Create_extension(
>    api->cancelation_requested = 0;
>    api->cancelability_state = PTHREAD_CANCEL_ENABLE;
>    api->cancelability_type = PTHREAD_CANCEL_DEFERRED;
> -  _Chain_Initialize_empty (&api->Cancellation_Handlers);
> +  api->last_cleanup_context = NULL;
>
>    /*
>     *  If the thread is not a posix thread, then all posix signals are blocked
> diff --git a/testsuites/psxtests/psxcleanup/psxcleanup.c b/testsuites/psxtests/psxcleanup/psxcleanup.c
> index 568e897..123e766 100644
> --- a/testsuites/psxtests/psxcleanup/psxcleanup.c
> +++ b/testsuites/psxtests/psxcleanup/psxcleanup.c
> @@ -250,13 +250,6 @@ void *POSIX_Init(
>
>    sleep(1);
>
> -  /*************** ERROR CASES  ***************/
> -  puts("Call pthread_cleanup_push with NULL handler");
> -  pthread_cleanup_push(NULL, NULL);
> -
> -  puts("Call pthread_cleanup_pop with no push");
> -  pthread_cleanup_pop(1);
> -
>    /*************** END OF TEST *****************/
>    puts( "*** END OF POSIX CLEANUP TEST ***\n" );
>    rtems_test_exit(0);
> --
> 1.7.7
>
> _______________________________________________
> rtems-devel mailing list
> rtems-devel at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-devel



More information about the devel mailing list