[PATCH] Reference counting for file system locations

[Sebastian Huber]

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.

[...]
>>> +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.

[...]
>>> +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.