[PATCH 4/5] posix: add user-facing hooks for shm object management

Gedare Bloom gedare at rtems.org
Wed Aug 24 16:01:20 UTC 2016


On Tue, Aug 23, 2016 at 10:00 PM, Chris Johns <chrisj at rtems.org> wrote:
> On 18/08/2016 6:02 AM, Gedare Bloom wrote:
>>
>> ---
>>  cpukit/posix/Makefile.am                   |  2 +
>>  cpukit/posix/include/rtems/posix/shm.h     | 65
>> ++++++++++++++++++++++++-----
>>  cpukit/posix/include/rtems/posix/shmimpl.h | 11 +++++
>>  cpukit/posix/src/shmheap.c                 | 60
>> ++++++++++++++++++++++++++
>>  cpukit/posix/src/shmopen.c                 | 56 +++++--------------------
>>  cpukit/posix/src/shmwkspace.c              | 67
>> ++++++++++++++++++++++++++++++
>>  cpukit/sapi/include/confdefs.h             | 13 +++++-
>>  testsuites/psxtests/Makefile.am            |  2 +-
>>  testsuites/psxtests/configure.ac           |  1 +
>>  9 files changed, 219 insertions(+), 58 deletions(-)
>>  create mode 100644 cpukit/posix/src/shmheap.c
>>  create mode 100644 cpukit/posix/src/shmwkspace.c
>>
>> diff --git a/cpukit/posix/Makefile.am b/cpukit/posix/Makefile.am
>> index 5490401..1851535 100644
>> --- a/cpukit/posix/Makefile.am
>> +++ b/cpukit/posix/Makefile.am
>> @@ -103,8 +103,10 @@ libposix_a_SOURCES += src/munlock.c
>>  libposix_a_SOURCES += src/munmap.c
>>  libposix_a_SOURCES += src/posix_madvise.c
>>  libposix_a_SOURCES += src/shm.c
>> +libposix_a_SOURCES += src/shmheap.c
>>  libposix_a_SOURCES += src/shmopen.c
>>  libposix_a_SOURCES += src/shmunlink.c
>> +libposix_a_SOURCES += src/shmwkspace.c
>>
>>  ## MESSAGE_QUEUE_C_FILES
>>  libposix_a_SOURCES += src/mqueue.c src/mqueueclose.c \
>> diff --git a/cpukit/posix/include/rtems/posix/shm.h
>> b/cpukit/posix/include/rtems/posix/shm.h
>> index f3d7a91..eae0dcf 100644
>> --- a/cpukit/posix/include/rtems/posix/shm.h
>> +++ b/cpukit/posix/include/rtems/posix/shm.h
>> @@ -30,6 +30,13 @@ extern "C" {
>>   * Internal implementation support for POSIX shared memory.
>>   * @{
>>   */
>> +typedef struct POSIX_Shm_Object_operations POSIX_Shm_Object_operations;
>> +extern const POSIX_Shm_Object_operations _POSIX_Shm_Object_operations;
>> +typedef struct {
>> +  void                               *handle;
>> +  size_t                              size;
>> +  const POSIX_Shm_Object_operations  *ops;
>> +} POSIX_Shm_Object;
>>
>>  typedef struct {
>>     Objects_Control      Object;
>> @@ -37,15 +44,7 @@ typedef struct {
>>
>>     int                  reference_count;
>>
>> -   void                *shm_object;
>> -   size_t               shm_object_size;
>> -   void                *( *shm_object_allocate )( size_t size );
>> -   void                *( *shm_object_reallocate )(
>> -                          void *ptr,
>> -                          size_t curr_size,
>> -                          size_t new_size
>> -                        );
>> -   void                 ( *shm_object_free ) ( void *ptr );
>> +   POSIX_Shm_Object     shm_object;
>>
>>     uid_t                uid;
>>     gid_t                gid;
>> @@ -54,9 +53,55 @@ typedef struct {
>>     time_t               atime;
>>     time_t               mtime;
>>     time_t               ctime;
>> -
>>  } POSIX_Shm_Control;
>>
>
> It is not clear to me what is being returned as an 'int' with these
> functions?
>
>
>> +struct POSIX_Shm_Object_operations {
>> +  int   ( *object_create ) ( POSIX_Shm_Control *shm, size_t size );
>> +  int   ( *object_resize ) ( POSIX_Shm_Control *shm, size_t size );
>> +  int   ( *object_delete ) ( POSIX_Shm_Control *shm );
>> +  void *( *object_mmap   ) ( POSIX_Shm_Control *shm, size_t len, int
>> prot, int flags, off_t off );
>> +};
>> +
>> +extern int _POSIX_Shm_Object_create_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +);
>> +
>> +extern int _POSIX_Shm_Object_delete_from_workspace( POSIX_Shm_Control
>> *shm );
>> +
>> +extern int _POSIX_Shm_Object_resize_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +);
>> +
>> +extern void *_POSIX_Shm_Object_mmap_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t len,
>> +  int prot,
>> +  int flags,
>> +  off_t off
>> +);
>> +
>> +extern int _POSIX_Shm_Object_create_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +);
>> +
>> +extern int _POSIX_Shm_Object_delete_from_heap( POSIX_Shm_Control *shm );
>> +
>> +extern int _POSIX_Shm_Object_resize_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +);
>> +
>> +extern void *_POSIX_Shm_Object_mmap_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t len,
>> +  int prot,
>> +  int flags,
>> +  off_t off
>> +);
>> +
>>  /** @} */
>>
>>  #ifdef __cplusplus
>> diff --git a/cpukit/posix/include/rtems/posix/shmimpl.h
>> b/cpukit/posix/include/rtems/posix/shmimpl.h
>> index 2df4bd3..c012771 100644
>> --- a/cpukit/posix/include/rtems/posix/shmimpl.h
>> +++ b/cpukit/posix/include/rtems/posix/shmimpl.h
>> @@ -81,6 +81,17 @@ RTEMS_INLINE_ROUTINE POSIX_Shm_Control
>> *_POSIX_Shm_Get_by_name(
>>      error
>>    );
>>  }
>> +
>> +RTEMS_INLINE_ROUTINE void _POSIX_Shm_Update_mtime_ctime(
>> +  POSIX_Shm_Control *shm
>> +)
>> +{
>> +  struct timeval now;
>> +  gettimeofday( &now, 0 );
>> +  shm->mtime = now.tv_sec;
>> +  shm->ctime = now.tv_sec;
>> +}
>> +
>>  /** @} */
>>
>>  #ifdef __cplusplus
>> diff --git a/cpukit/posix/src/shmheap.c b/cpukit/posix/src/shmheap.c
>> new file mode 100644
>> index 0000000..4f6f105
>> --- /dev/null
>> +++ b/cpukit/posix/src/shmheap.c
>> @@ -0,0 +1,60 @@
>> +/**
>> + * @file
>> + */
>> +
>> +/*
>> + * Copyright (c) 2016 Gedare Bloom.
>> + *
>> + * The license and distribution terms for this file may be
>> + * found in the file LICENSE in this distribution or at
>> + * http://www.rtems.org/license/LICENSE.
>> + */
>> +
>> +#if HAVE_CONFIG_H
>> +#include "config.h"
>> +#endif
>> +
>> +#include <errno.h>
>> +#include <stdlib.h>
>> +#include <rtems/posix/shmimpl.h>
>> +
>> +int _POSIX_Shm_Object_create_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +)
>> +{
>> +  shm->shm_object.handle = malloc( size );
>> +  shm->shm_object.size = size;
>> +  return 0;
>
>
> Looking further down I see this is an error number of some form. Why not
> test the result of malloc rather than test the handle?
>
The malloc and realloc calls should be error checked and set an
appropriate error code. I don't think it is a good idea to provide a
void* return here to avoid some "pointer leak" of the handle, which
can change or might not even be a valid pointer e.g. for shared memory
implemented over a network or in a paravirtualized system.

>> +}
>> +
>> +int _POSIX_Shm_Object_delete_from_heap( POSIX_Shm_Control *shm )
>> +{
>> +  free( shm->shm_object.handle );
>> +  shm->shm_object.handle = NULL;
>> +  shm->shm_object.size = 0;
>> +  return 0;
>> +}
>> +
>> +int _POSIX_Shm_Object_resize_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +)
>> +{
>> +  shm->shm_object.handle = realloc( shm->shm_object.handle, size );
>> +  shm->shm_object.size = size;
>> +  return 0;
>
>
> realloc can fail.
>
Thanks. I threw the heap implementation together in a hurry. I will
fix when I get to adding mmap support.

>
>> +}
>> +
>> +void *_POSIX_Shm_Object_mmap_from_heap(
>> +  POSIX_Shm_Control *shm,
>> +  size_t len,
>> +  int prot,
>> +  int flags,
>> +  off_t off
>> +)
>> +{
>> +  return NULL;
>> +}
>> +
>> +
>> diff --git a/cpukit/posix/src/shmopen.c b/cpukit/posix/src/shmopen.c
>> index c8284e6..7823785 100644
>> --- a/cpukit/posix/src/shmopen.c
>> +++ b/cpukit/posix/src/shmopen.c
>> @@ -28,55 +28,21 @@
>>
>>  static const rtems_filesystem_file_handlers_r shm_handlers;
>>
>> -static void* shm_reallocate_from_workspace(
>> -  void *ptr,
>> -  size_t curr_size,
>> -  size_t new_size
>> -)
>> -{
>> -  char *new_ptr;
>> -  char *old_ptr = (char*) ptr;
>> -  int i;
>> -
>> -  if ( new_size == 0 ) {
>> -    _Workspace_Free( ptr );
>> -    return NULL;
>> -  }
>> -
>> -  new_ptr = _Workspace_Allocate_or_fatal_error( new_size );
>> -  /* memcpy ... */
>> -  for ( i = 0; i < curr_size && i < new_size; i++ ) {
>> -    new_ptr[i] = old_ptr[i];
>> -  }
>> -  _Workspace_Free(ptr);
>> -
>> -  return new_ptr;
>> -}
>> -
>> -static inline void shm_mtime_ctime_update( POSIX_Shm_Control *shm )
>> -{
>> -  struct timeval now;
>> -
>> -  gettimeofday( &now, 0 );
>> -  shm->mtime = now.tv_sec;
>> -  shm->ctime = now.tv_sec;
>> -}
>> -
>>  static int shm_ftruncate( rtems_libio_t *iop, off_t length )
>>  {
>>    Thread_queue_Context  queue_context;
>>    POSIX_Shm_Control *shm = (POSIX_Shm_Control *) iop->data1;
>> +  int err;
>>
>>    _POSIX_Shm_Acquire_critical( shm, &queue_context );
>>
>> -  shm->shm_object = (*shm->shm_object_reallocate)(
>> -      shm->shm_object,
>> -      shm->shm_object_size,
>> -      length
>> -  );
>> -  shm->shm_object_size = length;
>> +  err = (*shm->shm_object.ops->object_resize)( shm, length );
>
>
> Is the handle tested or should the err value be set in the handlers?
>
> It would be nice to have what expected clearly documented.
>
I will write some better documentation. The shm_object.handle should
not be accessed directly, it is "private" to the shm_object
implementation with the exception that NULL and 0 size means no object
is allocated.

>
>>
>> -  shm_mtime_ctime_update( shm );
>> +  if ( err != 0 ) {
>> +    rtems_set_errno_and_return_minus_one( err );
>> +  }
>> +
>> +  _POSIX_Shm_Update_mtime_ctime( shm );
>>
>>    _POSIX_Shm_Release( shm, &queue_context );
>>    return 0;
>> @@ -110,11 +76,9 @@ static inline POSIX_Shm_Control *shm_allocate(
>>    gettimeofday( &tv, 0 );
>>
>>    shm->reference_count = 1;
>> -  shm->shm_object = NULL;
>> -  shm->shm_object_allocate = _Workspace_Allocate_or_fatal_error;
>> -  shm->shm_object_reallocate = shm_reallocate_from_workspace;
>> -  shm->shm_object_free = _Workspace_Free;
>> -  shm->shm_object_size = 0;
>> +  shm->shm_object.handle = NULL;
>> +  shm->shm_object.size = 0;
>> +  shm->shm_object.ops = &_POSIX_Shm_Object_operations;
>>    shm->mode = mode & ~rtems_filesystem_umask;
>>    shm->uid = geteuid();
>>    shm->gid = getegid();
>> diff --git a/cpukit/posix/src/shmwkspace.c b/cpukit/posix/src/shmwkspace.c
>> new file mode 100644
>> index 0000000..f4f79a1
>> --- /dev/null
>> +++ b/cpukit/posix/src/shmwkspace.c
>> @@ -0,0 +1,67 @@
>> +/**
>> + * @file
>> + */
>> +
>> +/*
>> + * Copyright (c) 2016 Gedare Bloom.
>> + *
>> + * The license and distribution terms for this file may be
>> + * found in the file LICENSE in this distribution or at
>> + * http://www.rtems.org/license/LICENSE.
>> + */
>> +
>> +#if HAVE_CONFIG_H
>> +#include "config.h"
>> +#endif
>> +
>> +#include <errno.h>
>> +#include <rtems/score/wkspace.h>
>> +#include <rtems/posix/shmimpl.h>
>> +
>> +int _POSIX_Shm_Object_create_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +)
>> +{
>> +  shm->shm_object.handle = _Workspace_Allocate_or_fatal_error( size );
>> +  shm->shm_object.size = size;
>> +  return 0;
>> +}
>> +
>> +int _POSIX_Shm_Object_delete_from_workspace( POSIX_Shm_Control *shm )
>> +{
>> +  _Workspace_Free( shm->shm_object.handle );
>> +  shm->shm_object.handle = NULL;
>> +  shm->shm_object.size = 0;
>> +  return 0;
>> +}
>> +
>> +int _POSIX_Shm_Object_resize_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t size
>> +)
>> +{
>> +  int err;
>> +
>> +  if ( size == 0 ) {
>> +    err = _POSIX_Shm_Object_delete_from_workspace( shm );
>> +  } else if ( shm->shm_object.handle == NULL && shm->shm_object.size == 0
>> ) {
>> +    err = _POSIX_Shm_Object_create_from_workspace( shm, size );
>> +  } else {
>> +    err = EIO;
>> +  }
>> +  return err;
>> +}
>
>
> Ah ok some this one does set a return code but uses handle checks.
>
> Chris
>
>
>> +
>> +void *_POSIX_Shm_Object_mmap_from_workspace(
>> +  POSIX_Shm_Control *shm,
>> +  size_t len,
>> +  int prot,
>> +  int flags,
>> +  off_t off
>> +)
>> +{
>> +  return NULL;
>> +}
>> +
>> +
>> diff --git a/cpukit/sapi/include/confdefs.h
>> b/cpukit/sapi/include/confdefs.h
>> index 385bf62..597c2f3 100644
>> --- a/cpukit/sapi/include/confdefs.h
>> +++ b/cpukit/sapi/include/confdefs.h
>> @@ -2658,6 +2658,17 @@ extern rtems_initialization_tasks_table
>> Initialization_tasks[];
>>     */
>>    #if !defined(CONFIGURE_MAXIMUM_POSIX_SHMS)
>>      #define CONFIGURE_MAXIMUM_POSIX_SHMS 0
>> +  #else
>> +    #ifdef CONFIGURE_INIT
>> +      #if !defined(CONFIGURE_HAS_OWN_POSIX_SHM_OBJECT_OPERATIONS)
>> +        const POSIX_Shm_Object_operations _POSIX_Shm_Object_operations =
>> {
>> +          _POSIX_Shm_Object_create_from_workspace,
>> +          _POSIX_Shm_Object_resize_from_workspace,
>> +          _POSIX_Shm_Object_delete_from_workspace,
>> +          _POSIX_Shm_Object_mmap_from_workspace
>> +        };
>> +      #endif
>> +    #endif
>>    #endif
>>
>>    /**
>> @@ -2667,7 +2678,7 @@ extern rtems_initialization_tasks_table
>> Initialization_tasks[];
>>     * This is an internal parameter.
>>     */
>>    #define CONFIGURE_MEMORY_FOR_POSIX_SHMS(_shms) \
>> -    _Configure_Object_RAM(_shms, sizeof(POSIX_Shm_Control) )
>> +    _Configure_POSIX_Named_Object_RAM(_shms, sizeof(POSIX_Shm_Control) )
>>
>>
>>    #ifdef CONFIGURE_POSIX_INIT_THREAD_TABLE
>> diff --git a/testsuites/psxtests/Makefile.am
>> b/testsuites/psxtests/Makefile.am
>> index 206c450..bc76339 100644
>> --- a/testsuites/psxtests/Makefile.am
>> +++ b/testsuites/psxtests/Makefile.am
>> @@ -9,7 +9,7 @@ _SUBDIRS += psxhdrs psx01 psx02 psx03 psx04 psx05 psx06
>> psx07 psx08 psx09 \
>>      psxcancel psxcancel01 psxclassic01 psxcleanup psxcleanup01 \
>>      psxconcurrency01 psxcond01 psxcond02 psxconfig01 psxenosys \
>>      psxitimer psxmsgq01 psxmsgq02 psxmsgq03 psxmsgq04 \
>> -    psxmutexattr01 psxobj01 psxrwlock01 psxsem01 psxshm01 \
>> +    psxmutexattr01 psxobj01 psxrwlock01 psxsem01 psxshm01 psxshm02 \
>>      psxsignal01 psxsignal02 psxsignal03 psxsignal04 psxsignal05
>> psxsignal06 \
>>      psxspin01 psxspin02 psxsysconf \
>>      psxtime psxtimer01 psxtimer02 psxualarm psxusleep psxfatal01
>> psxfatal02 \
>> diff --git a/testsuites/psxtests/configure.ac
>> b/testsuites/psxtests/configure.ac
>> index e150fc0..fa730da 100644
>> --- a/testsuites/psxtests/configure.ac
>> +++ b/testsuites/psxtests/configure.ac
>> @@ -191,6 +191,7 @@ psxrdwrv/Makefile
>>  psxrwlock01/Makefile
>>  psxsem01/Makefile
>>  psxshm01/Makefile
>> +psxshm02/Makefile
>>  psxsignal01/Makefile
>>  psxsignal02/Makefile
>>  psxsignal03/Makefile
>>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel


More information about the devel mailing list