[PATCH] Reference counting for file system locations

[Gedare Bloom]

On Thu, Mar 1, 2012 at 8:25 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> Hello Gedare,
>
> thanks again.
>
> http://git.rtems.org/sebh/rtems.git/commit/?h=fsrefcount&id=a1def3107c8f8552798383d2c9b2d3ba9724431b
>
>
> On 02/28/2012 05:49 PM, Gedare Bloom wrote:
>>
>> Looks good, just a few more responses.
>>
>> On Tue, Feb 28, 2012 at 5:43 AM, Sebastian Huber
>> <sebastian.huber at embedded-brains.de>  wrote:
>
> [...]
>
>>>> +#define rtems_filesystem_mt_entry_lock_context( ctx )
>>>> rtems_interrupt_level ctx
>>>> I found this confusing because it is a declaration and not a function
>>>> call
>>>> Maybe rtems_filesystem_mt_entry_declare_lock( ctx )
>>>
>>>
>>>
>>> On a SMP configuration this may change from interrupt disable/enable to
>>> something else.
>>>
>> My complaint was more that at first glance I thought that
>> mt_entry_lock_context is grabbing the lock, when in fact it just is
>> declaring the context for a lock. Even in SMP this would be a
>> declaration of some handle for locking right? so I still think
>> rtems_filesystem_mt_entry_declare_lock_context(ctx) makes more sense.
>
>
> Fixed.
>
> [...]
>
>>>> +rtems_filesystem_global_location_t
>>>> *rtems_filesystem_global_location_transform(
>>>> +  rtems_filesystem_location_info_t *src
>>>> +);
>>>> maybe rename to filesystem_location_transform_to_global() or similar?
>>>
>>>
>>>
>>> I like this name, but most other operations with global locations start
>>> with
>>> rtems_filesystem_global_location_*.
>>>
>> The problem I have with the current name is that the argument to the
>> function is not a global location, but is instead just a location. The
>> other operations that start with rtems_filesystem_global_locatioon_*
>> take as argument a global location.
>
>
> Fixed.
>
Now its declaration is two characters > 80 in libio_.h:437

> [...]
>
>>>> +static inline bool rtems_filesystem_is_directory_up(
>>>> +  const char *token,
>>>> +  size_t tokenlen
>>>> +)
>>>> maybe rtems_filesystem_is_parent_directory()?
>>>
>>>
>>>
>>> Not sure.
>
>
> Fixed.
>
>
>>>
>>>
>>>>
>>>>
>>>> +++ b/cpukit/libcsupport/src/__usrenv.c
>>>> should the null handlers defined here set the errno?
>>>
>>>
>>>
>>> No! They must not touch the errno.  Some error will set the errno and the
>>> purpose of the null handlers is to deliver the error return status.
>>>
>> Is this documented somewhere? Maybe it is clear to others but it was
>> not clear to me when reading the code.
>
>
> Fixed.
>
>
>>
>>>
>>>>
>>>>
>>>> +rtems_filesystem_global_location_t
>>>> rtems_filesystem_global_location_null
>>>> = {
>>>> +  .location = {
>>>> +    .mt_entry_node = {
>>>> +      .next =&rtems_filesystem_null_mt_entry.location_chain.Tail.Node,
>>>> +      .previous
>>>> =&rtems_filesystem_null_mt_entry.location_chain.Head.Node
>>>> +    },
>>>> +    .handlers =&rtems_filesystem_null_handlers,
>>>> +    .ops =&null_ops,
>>>> +    .mt_entry =&rtems_filesystem_null_mt_entry
>>>> +  },
>>>> +  .reference_count = 4
>>>> +};
>>>> why reference_count 4?
>>>
>>>
>>>
>>> The current and root directory in the environment (2), the root node of
>>> the
>>> null file system instance (1), and the mount point node (1).
>>>
>> If it is ever possible this will change then this explanation should
>> be given in the code.
>
>
> 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.