fs ops was Re: change log for rtems (2010-06-22)

[Chris Johns]

On 24/06/10 12:09 PM, Joel Sherrill wrote:
> On 06/23/2010 05:57 PM, Chris Johns wrote:
>> On 24/06/10 1:05 AM, Sebastian Huber wrote:
>>> On 06/23/2010 04:57 PM, Joel Sherrill wrote:
>>>> On 06/23/2010 07:23 AM, Sebastian Huber wrote:
>>> [...]
>>>>> We should consider to forbid NULL pointers for ops tables and
>>>>> handlers.
>>>>> Instead we should provide reasonable default handlers. This would
>>>>> eliminate a
>>>>> lot of ifs and make the code more readable.
>>>>>
>>>>>
>>>> I was thinking of the same thing. There are a LOT of places there
>>>> are checks and having standard stubs would really help the code.
>>> [...]
>>>
>>> There are also a lot of places which don't check this and you always
>>> have to
>>> think about why this is justified in that place.
>>>
>> Agreed. This could be looked at when we make this area of code thread
>> safe. I suspect a few things will need to change to get the code thread
>> safe.
> Jennifer already suggested that some of the callouts are required
> by anything vaguely pretending to be a functional filesystem. The
> eval handlers come to mind on these.

This is sensible and I am not in disagreement. Looking at each file 
system I see DOSFS and NFS use the free call to clean up. The IMFS and 
RFS define handlers but they are empty.

Originally I thought the RFS did not provide this handler and so did not 
have the call happening. I am mistaken. The RFS does not use any info 
space outside the pathloc itself and so does not need a freeinfo 
handler. I must have missed removing it.

As I said I think making all this code thread safe is going to require 
us to have a close look at all of this.

> She is working on moving the filesystem operations to stubs. As
> part of that, she will note which appear to be "really really" needed
> as opposed to those are optional like write operations, symlinks,
> etc.

Great. Will she remove those handlers that are the same as the stubs and 
replace with a generic stubs ? The IMFS and RFS freeinfo handler is an 
example.

> But we will be watching the impact on size. My take is that there
> is enough conditional logic to be removed where this is a wash
> on the smaller apps and a win on larger ones.

I wait with interest for the result. :)

> Would it be acceptable to have one stub that returned ENOTSUP
> and define macros that cast that as the proper indirect method type?
> Something like this would let us have one stub and a clear reduction
> in code space.
>
> int rtems_filesystem_operations_stub(void)
> {
> rtems_set_errno_and_return_minus_one(ENOTSUP);
> }
>
> #define rtems_filesystem_default_read \
> (rtems_filesystem_read_t) rtems_filesystem_operations_stub
> #define rtems_filesystem_default_write \
> (rtems_filesystem_write_t) rtems_filesystem_operations_stub

I do not know. We should use a macro to abstract the way we implement 
this. I always feel not lying to the compiler is best.

Could the file system register or mount functions scan the handler table 
and reject a mount or file system registration that does not have the 
required handlers ?

> Then the operations table get the name defined in the macro
> and still hit the single stub.
>
> That would work for all that return int. Another for each return
> type (e.g. another for lseek_t since it returns an rtems_off64_t).
> The _rtems_filesystem_file_handlers_r has 14 elements and
> could be handled by 2 stubs. The _rtems_filesystem_operations_table
> has 18 more entries and could use the same stub.
>
> On the SPARC, this check is about 8-10 instructions each time which
> would be eliminated.

I really do not know if this is an ok thing to do.

Chris