fs core fixes (was Re: TFTP driver changes)
Till Straumann
strauman at SLAC.Stanford.EDU
Wed Sep 26 22:40:32 UTC 2001
Joel Sherrill wrote:
> Just an FYI since Jennifer and I both must agree on filesystem core changes
> before they are approved. She wrote most of the code based on a prototype
> I did. She is in the hospital this week with exhaustion. When she gets
> back
> to work, I will discuss the odd cases with her.
>
> Just like confdefs.h, the IMFS started life as a test fixture and ended up
> being useful. We did recognize the benefit of the IMFS earlier though. :)
> Regardless, the error handling you pointed out needs to be checked by
> Jennifer and I. If it is broken as you state, Jennifer will work her
> magic.
There are two issues here:
1) some necessary changes in the TFTPfs implementation to make relative
paths work. (Eric is taking care of this)
2) While working on a couple of suggestions for Eric, I found a couple
of what I think are bugs in the filesystem core (not IMFS) code. They are
mostly related to properly taking care of node cleanup (which is not used
by IMFS, so far).
I would greatly appreciate if Jennifer could analyze the problems I will
describe in this message. (I'm not trying to be pushy here - she certainly
should take care of herself and get well, first - I just send this now that
I've stuck my nose in, so it's off my mind and I can revert to doing other stuff...)
OK, now regarding the 'node_access' field in the rtems_filesystem_location_info_t
structure:
My assumption is that this field is for use by a particular filesystem implementation.
Because the filesystem implementation may attach dynamically allocated memory
(or other resources) to 'node_access', both, the fs implementation (fsi) and the
framework provided by the filesystem core (coded in libc) must adhere to a common
policy regarding node allocation and cleanup.
>From studying the source, it seems to me that the paradigm is that it's the job
of the 'evaluate and mount methods' (evalpath_h and evalformake_h
as well as fsmount_me_h for initializing the mounted fs root node) to initialize the
node_access field if it's going to be used by other fsi routines. However - the
core must ensure the necessary calls of rtems_filesystem_freenode().
It seems that rtems_filesystem_freenode() _must_ be called on any
nodes which yielded a successful evalpath_h(). Conversely, freenode()
_must_not_ be called on any node for which evalpath() failed since the
node can not be regarded as properly initialized.
The appended patch fixes all deviations from this policy I found in libc.
There are two more complex cases, however: open and unmount.
1) OPEN: a successful open allocates a rtems_libio_t structure and copies
the node which was obtained by a previous 'evalpath_h' into the libio
structure. IMHO, in this case (i.e. a _successful_ open), the node _must_not_
be released because the node_access pointer copied into the libio struct
could still be referenced in the future (by any fsi code operating on the
open fd). Instead, rtems_filesystem_freenode() must be called by 'close'.
2) There is another issue with MOUNT/UMOUNT: IMHO, the verification if the
target node is the fs root node is not quite useful for two reasons:
- a fs might not use node_access and set it to NULL for any node
which would identify any node as the fs root node.
- a fs using node_access might attach equivalent but not identical
objects to node access (i.e. copies of the same string) to node_access
which would cause the verification to fail although the target
node _is_ indeed the fs root node.
Possible solutions could be:
a) (umount only) comparing all the other
fields (but node_access) leading to slightly different semantics for umount
(unmount a fs by referencing any file on the fs).
b) adding a flag to filesystem_location_info_t identifying
a fs root node.
Thinking this over a little bit, I guess the current behavior might be
acceptible. However, fs implementors should set the node_access
field of the fs root to some special value thereby tagging it
as the root node; on any other node, the fsi must set node_access to
some other value - a not very clean solution.
Other ideas??
Note that the attached patch has not been thoroughly tested and needs
inspection by a fs expert.
Regards, (sorry for the long message)
-- Till.
-------------- next part --------------
*** c/src/lib/libc/close.c.orig Wed Sep 26 15:22:12 2001
--- c/src/lib/libc/close.c Wed Sep 26 15:23:58 2001
***************
*** 32,37 ****
--- 32,41 ----
if ( iop->handlers->close_h )
rc = (*iop->handlers->close_h)( iop );
+ /* T. Straumann, 9/25/2001: free node contained in
+ * libio structure
+ */
+ rtems_filesystem_freenode( &iop->pathinfo );
rtems_libio_free( iop );
return rc;
*** c/src/lib/libc/link.c.orig Wed Sep 26 14:56:15 2001
--- c/src/lib/libc/link.c Wed Sep 26 14:58:41 2001
***************
*** 48,61 ****
if ( !parent_loc.ops->evalformake_h ) {
rtems_filesystem_freenode( &existing_loc );
! rtems_filesystem_freenode( &parent_loc );
set_errno_and_return_minus_one( ENOTSUP );
}
result = (*parent_loc.ops->evalformake_h)( &new[i], &parent_loc, &name_start );
if ( result != 0 ) {
rtems_filesystem_freenode( &existing_loc );
! rtems_filesystem_freenode( &parent_loc );
set_errno_and_return_minus_one( result );
}
--- 48,65 ----
if ( !parent_loc.ops->evalformake_h ) {
rtems_filesystem_freenode( &existing_loc );
! /* T. Straumann, 9/25/2001, must not free parent_loc until eval succeeds
! * rtems_filesystem_freenode( &parent_loc );
! */
set_errno_and_return_minus_one( ENOTSUP );
}
result = (*parent_loc.ops->evalformake_h)( &new[i], &parent_loc, &name_start );
if ( result != 0 ) {
rtems_filesystem_freenode( &existing_loc );
! /* T. Straumann, 9/25/2001, must not free parent_loc until eval succeeds
! * rtems_filesystem_freenode( &parent_loc );
! */
set_errno_and_return_minus_one( result );
}
*** c/src/lib/libc/open.c.orig Wed Sep 26 15:13:58 2001
--- c/src/lib/libc/open.c Wed Sep 26 15:22:07 2001
***************
*** 65,71 ****
int rc;
rtems_libio_t *iop = 0;
int status;
! rtems_filesystem_location_info_t loc;
int eval_flags;
--- 65,71 ----
int rc;
rtems_libio_t *iop = 0;
int status;
! rtems_filesystem_location_info_t loc, *loc_to_free=NULL;
int eval_flags;
***************
*** 138,146 ****
--- 138,150 ----
} else if ((flags & (O_EXCL|O_CREAT)) == (O_EXCL|O_CREAT)) {
/* We were trying to create a file that already exists */
rc = EEXIST;
+ loc_to_free = &loc;
goto done;
}
+ loc_to_free = &loc;
+
+
/*
* Fill in the file control block based on the loc structure
* returned by successful path evaluation.
***************
*** 178,187 ****
if ( rc ) {
if ( iop )
rtems_libio_free( iop );
set_errno_and_return_minus_one( rc );
}
-
- rtems_filesystem_freenode( &loc );
return iop - rtems_libio_iops;
}
--- 182,191 ----
if ( rc ) {
if ( iop )
rtems_libio_free( iop );
+ if ( loc_to_free )
+ rtems_filesystem_freenode( loc_to_free );
set_errno_and_return_minus_one( rc );
}
return iop - rtems_libio_iops;
}
*** c/src/lib/libc/unmount.c.orig Wed Sep 26 15:00:13 2001
--- c/src/lib/libc/unmount.c Wed Sep 26 15:13:45 2001
***************
*** 128,133 ****
--- 128,137 ----
*/
rtems_filesystem_freenode( &fs_root_loc.mt_entry->mt_point_node );
+ /* T. Straumann, 9/25/2001: must release the fs root node also - or
+ * should this be the job of fsunmount_me ??
+ */
+ rtems_filesystem_freenode( &fs_root_loc.mt_entry->mt_fs_root );
free( fs_root_loc.mt_entry );
rtems_filesystem_freenode( &fs_root_loc );
***************
*** 168,175 ****
*/
*fs_to_unmount = *fs_root_loc->mt_entry;
! if ( fs_to_unmount->mt_fs_root.node_access != fs_root_loc->node_access )
set_errno_and_return_minus_one( EACCES );
/*
* Search the mount table for any mount entries referencing this
--- 172,182 ----
*/
*fs_to_unmount = *fs_root_loc->mt_entry;
! if ( fs_to_unmount->mt_fs_root.node_access != fs_root_loc->node_access ) {
! /* T. Straumann, 9/25/2001: must release fs_root_loc */
! rtems_filesystem_freenode(fs_root_loc);
set_errno_and_return_minus_one( EACCES );
+ }
/*
* Search the mount table for any mount entries referencing this
***************
*** 181,186 ****
--- 188,195 ----
the_node = the_node->next ) {
the_mount_entry = ( rtems_filesystem_mount_table_entry_t * )the_node;
if (the_mount_entry->mt_point_node.mt_entry == fs_root_loc->mt_entry ) {
+ /* T. Straumann, 9/25/2001: must release fs_root_loc */
+ rtems_filesystem_freenode(fs_root_loc);
set_errno_and_return_minus_one( EBUSY );
}
}
*** c/src/lib/libc/utime.c.orig Wed Sep 26 14:59:11 2001
--- c/src/lib/libc/utime.c Wed Sep 26 14:59:57 2001
***************
*** 32,39 ****
if ( rtems_filesystem_evaluate_path( path, 0x00, &temp_loc, TRUE ) )
return -1;
! if ( !temp_loc.ops->utime_h )
set_errno_and_return_minus_one( ENOTSUP );
result = (*temp_loc.ops->utime_h)( &temp_loc, times->actime, times->modtime );
--- 32,42 ----
if ( rtems_filesystem_evaluate_path( path, 0x00, &temp_loc, TRUE ) )
return -1;
! if ( !temp_loc.ops->utime_h ) {
! /* T. Straumann, 9/25/2001; must release temp_loc */
! rtems_filesystem_freenode( &temp_loc );
set_errno_and_return_minus_one( ENOTSUP );
+ }
result = (*temp_loc.ops->utime_h)( &temp_loc, times->actime, times->modtime );
More information about the users
mailing list