On Tue, Aug 23, 2016 at 10:00 PM, Chris Johns <chr...@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@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
