[PATCH] Reference counting for file system locations

[Sebastian Huber]

On 03/01/2012 03:46 PM, Gedare Bloom wrote:
> On Thu, Mar 1, 2012 at 8:25 AM, Sebastian Huber
> <sebastian.huber at embedded-brains.de>  wrote:
[...]
> Now its declaration is two characters>  80 in libio_.h:437

Fixed.
[...]
>>>>> +++ b/cpukit/libcsupport/src/freenode.c
>>>>> +void rtems_filesystem_freenode( rtems_filesystem_location_info_t *node
>>>>> )
>>>>> Might be worth calling this rtems_filesystem_location_free()?
>>>>
>>>>
>>>>
>>>> Hm, this name is historic.  Not sure.
>>>>
>>> Maybe someone else can chime in, or we can bring it up later. Seems
>>> like we could unify the naming to replace the idea of a filesystem
>>> 'node' with a filesystem location.
>>
>>
>> Fixed.
>>
> Should we rename the files freenode.c and clonenode.c or maybe even
> move these functions into sup_fs_location.c?
>
> There are also a few residual places where handlers use
> clonenode/freenode as part of their name; I am unsure whether it is
> better to have the freenode/clonenode names on these handlers or not,
> since the function pointer in the op table is still called
> freenod_h/clonenod_h. However, the argument to those functions is a
> filesystem location so I think it makes sense, and we should consider
> renaming freenod_h/clonenod_h---although they are consistent with
> other function handlers like mknod_h/rmnod_h so I don't know. Renaming
> these two functions may have opened a can of worms.
>
> grep -r clonenode *
>
> libcsupport/src/__usrenv.c:static int null_op_clonenode(
> libcsupport/src/__usrenv.c:  .clonenod_h = null_op_clonenode,
> libcsupport/Makefile.am:    src/clonenode.c \
> libcsupport/include/rtems/libio.h: * @see rtems_filesystem_default_clonenode().
> libcsupport/include/rtems/libio.h:typedef int (*rtems_filesystem_clonenode_t)(
> libcsupport/include/rtems/libio.h:  rtems_filesystem_clonenode_t clonenod_h;
> libcsupport/include/rtems/libio.h: * @see rtems_filesystem_clonenode_t.
> libcsupport/include/rtems/libio.h:int rtems_filesystem_default_clonenode(
> libfs/src/defaults/default_ops.c:  .clonenod_h =
> rtems_filesystem_default_clonenode,
> libfs/src/defaults/default_clone.c:int rtems_filesystem_default_clonenode(
> libfs/src/rfs/rtems-rfs-rtems.c:  .clonenod_h     =
> rtems_filesystem_default_clonenode,
> libfs/src/nfsclient/src/nfs.c:static int
> nfs_clonenode(rtems_filesystem_location_info_t *loc)
> libfs/src/nfsclient/src/nfs.c:	.clonenod_h     = nfs_clonenode,
> libfs/src/devfs/devfs_init.c:  .clonenod_h = rtems_filesystem_default_clonenode,
> libnetworking/lib/ftpfs.c:  .clonenod_h = rtems_filesystem_default_clonenode,
>
> grep -r freenode *
>
> libcsupport/src/freenode.c: *  freenode()
> libcsupport/src/__usrenv.c:  .freenod_h = rtems_filesystem_default_freenode,
> libcsupport/Makefile.am:    src/freenode.c \
> libcsupport/include/rtems/libio.h: * @see rtems_filesystem_default_freenode().
> libcsupport/include/rtems/libio.h:typedef void (*rtems_filesystem_freenode_t)(
> libcsupport/include/rtems/libio.h:  rtems_filesystem_freenode_t freenod_h;
> libcsupport/include/rtems/libio.h: * @see rtems_filesystem_freenode_t.
> libcsupport/include/rtems/libio.h:void rtems_filesystem_default_freenode(
> libcsupport/include/rtems/libio_.h: * @see rtems_filesystem_freenode_t.
> libfs/src/defaults/default_ops.c:  .freenod_h =
> rtems_filesystem_default_freenode,
> libfs/src/defaults/default_freenode.c:void rtems_filesystem_default_freenode(
> libfs/src/rfs/rtems-rfs-rtems.c:  .freenod_h      =
> rtems_filesystem_default_freenode,
> libfs/src/nfsclient/src/nfs.c:static void nfs_freenode(
> libfs/src/nfsclient/src/nfs.c:			"entering freenode, in use count is
> %i nodes, %i strings\n",
> libfs/src/nfsclient/src/nfs.c:	.freenod_h      = nfs_freenode,
> libfs/src/nfsclient/src/nfs.c:		/* must restore the cwd because
> 'freenode' will be called on it */
> libfs/src/devfs/devfs_init.c:  .freenod_h = rtems_filesystem_default_freenode,
> libfs/Makefile.am:    src/defaults/default_fchmod.c
> src/defaults/default_freenode.c \
>
> There are also these:
> dosfs/msdos_init.c:43:  .clonenod_h     =  msdos_clone_node_info,
> imfs/fifoimfs_init.c:39:  .clonenod_h = IMFS_node_clone,
> imfs/miniimfs_init.c:37:  .clonenod_h = IMFS_node_clone,
> imfs/imfs_init.c: 37:  .clonenod_h = IMFS_node_clone,
>
> dosfs/msdos_init.c:44:  .freenod_h      =  msdos_free_node_info,
> imfs/fifoimfs_init.c:40:  .freenod_h = IMFS_node_free,
> imfs/miniimfs_init.c:38:  .freenod_h = IMFS_node_free,
> imfs/imfs_init.c:38:  .freenod_h = IMFS_node_free
>
>> [...]
>>
>>>>> +static void free_user_env(void *arg)
>>>>> ...
>>>>> +  /*
>>>>> +   * It is possible to see the global environment here if someone
>>>>> deletes
>>>>> the
>>>>> +   * task right after the task variable add in
>>>>> rtems_libio_set_private_env().
>>>>> +   */
>>>>> make it more clear this is protected by thread dispatching disable
>>>>
>>>>
>>>>
>>>> What is protected?
>>>>
>>> Sorry I misread this part, and re-reading it is confusing me a bit
>>> more. Is this comment saying there is a race condition here that can
>>> cause free_user_env to get passed the old_env (which might be the
>>> global_env) when the task is deleted before set_private_env is able to
>>> update the rtems_current_user_env? If so what does this mean? If I
>>> read this code right, it means the new_env is leaked.
>>
>>
>> Yes, you are right, this results in a resource leak.  I removed the comment.
>> To delete a running thread is always dangerous.
>>
>>
>>>
>>> What about if the old_env is a shared environment that is not the
>>> global environment? Would the same race condition exist and could it
>>> cause the old_env to be freed prematurely from underneath the other
>>> task sharing the old_env?
>>
>>
>> [...]
>>
>>
>> --
>> Sebastian Huber, embedded brains GmbH
>>
>> Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
>> Phone   : +49 89 18 90 80 79-6
>> Fax     : +49 89 18 90 80 79-9
>> E-Mail  : sebastian.huber at embedded-brains.de
>> PGP     : Public key available on request.
>>
>> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.


-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.