[PATCH] Reference counting for file system locations
Gedare Bloom
gedare at rtems.org
Mon Feb 27 21:28:18 UTC 2012
On Sat, Feb 18, 2012 at 3:25 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> On 17/02/12 17:39, Sebastian Huber wrote:
>>
>> Hello,
>>
>> sorry that this patch is so huge, but it is hard to get it smaller since
>> the introduction of reference counting was quite a massive undertaking.
>>
>> Please review and test this patch. A fix for the file system in
>> tfsDriver.c is not included (I will do this after the review).
>>
>> Have a nice day!
>>
>
> Somehow the git send-email
> 0001-Reference-counting-for-file-system-locations.patch didn't work. The
> patch can be found here:
>
> http://git.rtems.org/sebh/rtems.git/commit/?h=fsrefcount
>
This looks like it was quite the undertaking, and it appears to have
been done well. I have read through the code, but I have not yet had a
chance to try it out. With any luck I can get a build going soon and
run the tests. I did not spend much time looking at the testsuites or
the individual fs code although I did skim them. Here are my
comments, some of which may be questions stemming from my own
not-understanding / curiosity:
+++ b/c/src/lib/libbsp/shared/umon/tfsDriver.c
+static void rtems_tfs_eval_path(rtems_filesystem_eval_path_context_t *self)
+ {
+ int eval_flags = rtems_filesystem_eval_path_get_flags(self);
+
+ if ((eval_flags & RTEMS_LIBIO_MAKE) == 0) {
+ eval_flags &= RTEMS_LIBIO_PERMS_READ | RTEMS_LIBIO_PERMS_WRITE;
+ if (
+ eval_flags == 0
+ || eval_flags == RTEMS_LIBIO_PERMS_READ
+ || eval_flags == RTEMS_LIBIO_PERMS_WRITE
Is it an error to have both read and write? That is the only 'else' case,
so this looks equivalent to if ( eval_flags != RTEMS_LIBIO_PERMS_RDW )
.. otherwise
something odd is going on here. Also, why doesn't this function use
the rtems_filesystem_eval_path_generic code?
@@ -671,68 +593,92 @@ static int rtems_tfs_ioctl(
+ if (ret != TFS_OKAY)
+ return(-1);
+
+ return(-1);
+}
this chunk always returns -1 so something seems wrong here.
+++ b/cpukit/include/rtems/fs.h
+typedef struct rtems_filesystem_global_location_t {
+ rtems_filesystem_location_info_t location;
+ int reference_count;
+ struct rtems_filesystem_global_location_t *deferred_released_next;
At first glance its not obvious deferred_released_next implements a simple fifo
maybe just add some more to the comment above about deferred release.
Why not using an rtems_chain to manage the deferred release nodes?
+++ b/cpukit/libcsupport/include/rtems/libio.h
+typedef struct {
+ const char *path;
+ size_t pathlen;
+ const char *token;
+ size_t tokenlen;
+ int flags;
+ int recursionlevel;
+ rtems_filesystem_location_info_t currentloc;
+ rtems_filesystem_global_location_t *rootloc;
+ rtems_filesystem_global_location_t *startloc;
+} rtems_filesystem_eval_path_context_t;
Might be nice to know a little more about these fields.
+ * @brief Changes the mode of a node.
+ *
+ * @param[in, out] iop The IO pointer.
+ * @param[in] mode The new mode of the node
+ *
+ * @retval 0 Successful operation.
+ * @retval -1 An error occured. The errno is set to indicate the error.
+ *
+ * @see rtems_filesystem_default_fchmod().
*/
+typedef int (*rtems_filesystem_fchmod_t)(
+ const rtems_filesystem_location_info_t *loc,
+ mode_t mode
);
outdated comment above?
s/@param[in, out] iop The IO pointer./@param[in] loc The location of the node./
+ * @brief Unmounts a file system instance in a mount point (directory).
+ *
+ * In case this function is successful the file system instance will be marked
+ * as unmounted. The file system instance will be destroyed then the last
s/then/when
+/**
+ * @brief File system operations table with default operations.
+ */
+extern const rtems_filesystem_operations_table
+rtems_filesystem_operations_default;
indent nested line?
+extern const rtems_filesystem_file_handlers_r
+rtems_filesystem_handlers_default;
indent?
-#define RTEMS_LIBIO_PERMS_READ S_IROTH
-#define RTEMS_LIBIO_PERMS_WRITE S_IWOTH
-#define RTEMS_LIBIO_PERMS_RDWR (S_IROTH|S_IWOTH)
-#define RTEMS_LIBIO_PERMS_EXEC S_IXOTH
+#define RTEMS_LIBIO_PERMS_READ 0x4
+#define RTEMS_LIBIO_PERMS_WRITE 0x2
+#define RTEMS_LIBIO_PERMS_EXEC 0x1
Why changing these away from the S_ versions?
Are they still compatible with mode_t bits?
Are these the values for the 'flags' field of eval_path_context_t?
+typedef struct {
+ const char *source;
+ const char *target;
+ const char *filesystemtype;
+ rtems_filesystem_options_t options;
+ const void *data;
+} rtems_filesystem_mount_configuration;
what do fields mean?
+++ b/cpukit/libcsupport/include/rtems/libio_.h
+void rtems_filesystem_clonenode(
+ rtems_filesystem_location_info_t *clone,
+ const rtems_filesystem_location_info_t *master
+);
What is the difference between filesystem_clonenode and filesystem_clone?
Maybe rename: filesystem_location_clone().
+#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 )
+typedef struct {
+ rtems_filesystem_eval_path_is_directory is_directory;
+ rtems_filesystem_eval_path_eval_token eval_token;
+} rtems_filesystem_eval_path_generic_config;
I think generic_operations is a more accurate name, or callouts,
or hooks, or something like that; config seems like it should be some
data (not functions) that determines what should be done. Maybe it's just me.
+rtems_filesystem_location_info_t *rtems_filesystem_location_initialize(
+ rtems_filesystem_location_info_t *dst,
+ const rtems_filesystem_location_info_t *src
+);
When I think of initialize I think of initializing to zero.
I would like to consider renaming this function to be location_copy
or location_clone. I would leave the location_initialize_to_null name as-is.
+static inline void rtems_filesystem_location_initialize_to_null(
are we supposed to use inline in cpukit? or RTEMS_INLINE_ROUTINE? Same
with rest of inline functions defined here.
+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?
Could use a couple comments about this function. May block; obtains newly
created global location. I notice that before this is called the src location
always is extracted from current_loc--does this imply some pre-requisite about
the src location?
+void rtems_filesystem_global_location_assign(
+ rtems_filesystem_global_location_t **lhs_global_loc_ptr,
+ rtems_filesystem_global_location_t *rhs_global_loc
+);
should rhs be const?
+void rtems_filesystem_replace_with_null_location(
+ rtems_filesystem_location_info_t *replace
+);
+
+void rtems_filesystem_get_and_replace_with_null_location(
+ rtems_filesystem_location_info_t *get,
+ rtems_filesystem_location_info_t *replace
+);
Many of the functions are named like rtems_filesystem_global_location_xxx
and rtems_filesystem_location_xxx. Consider whether to rename these two
functions to e.g. rtems_filesystem_location_replace_with_null/get_and_replace.
I also suggest rtems_filesystem_location_detach/copy_and_detach.
Replacing get with copy seems sensible if location_initialize is replaced
by location_copy.
+static inline void rtems_filesystem_add_location_to_mount_entry(
+ rtems_filesystem_location_info_t *loc
+)
+{
+ rtems_filesystem_mt_entry_lock_context( lock_context );
+
+ rtems_filesystem_mt_entry_lock( lock_context );
+ rtems_chain_append_unprotected(
+ &loc->mt_entry->location_chain,
+ &loc->mt_entry_node
+ );
+ rtems_filesystem_mt_entry_unlock( lock_context );
+}
+
+void rtems_filesystem_remove_location_from_mount_entry(
+ rtems_filesystem_location_info_t *loc
+);
I suggest rtems_filesystem_location_add_to_mt_entry/remove_from_mt_entry.
+static inline bool rtems_filesystem_is_root_node(
+ const rtems_filesystem_location_info_t *loc
+)
I suggest filesysytem_location_is_root?
+static inline void rtems_filesystem_eval_path_pass_back_token(
+ rtems_filesystem_eval_path_context_t *ctx
+)
oddly named function "pass_back_token". I might suggest "put" instead of
"pass" as more symmetrically named with eval_path_get_next_token. Since this is
just a static helper function the name is no big deal.
+int rtems_filesystem_exist_in_same_file_system_instance(
+ const rtems_filesystem_location_info_t *a,
+ const rtems_filesystem_location_info_t *b
+);
maybe rtems_filesystem_location_exists_in_same_fs_instance_as?
+static inline bool rtems_filesystem_is_directory_up(
+ const char *token,
+ size_t tokenlen
+)
maybe rtems_filesystem_is_parent_directory()?
+++ b/cpukit/libcsupport/src/__usrenv.c
should the null handlers defined here set the errno?
+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?
+++ b/cpukit/libcsupport/src/_rename_r.c
+ /* FIXME: This is not POSIX conform */
+ int new_eval_flags = RTEMS_LIBIO_FOLLOW_HARD_LINK
+ | RTEMS_LIBIO_MAKE
+ | RTEMS_LIBIO_EXCLUSIVE;
FIXME :) What would be conformant?
+++ b/cpukit/libcsupport/src/chdir.c
+int rtems_filesystem_chdir( rtems_filesystem_location_info_t *loc )
...
+ } else {
+ rv = -1;
+ }
+
+ if ( rv != 0 ) {
+ rtems_filesystem_global_location_release( global_loc );
+ }
set errno when setting rv = -1?
+++ b/cpukit/libcsupport/src/chroot.c
+ /*
+ * We use the global environment for path evaluation. This makes it possible
+ * to escape from a chroot environment referencing an unmounted file system.
+ */
clarify comment. Is this a problem that can/should be fixed?
+ sc = rtems_libio_set_private_env();
+ if (sc == RTEMS_SUCCESSFUL) {
+ rtems_filesystem_global_location_assign(
+ &rtems_filesystem_root,
+ new_root_loc
+ );
+ rtems_filesystem_global_location_assign(
+ &rtems_filesystem_current,
+ new_current_loc
+ );
+ } else {
+ if (sc != RTEMS_UNSATISFIED) {
+ errno = ENOMEM;
+ }
+ rv = -1;
+ }
what if sc is RTEMS_UNSATISFIED what should errno be?
...
+ } else {
+ rv = -1;
set errno?
+ }
+ rtems_filesystem_eval_path_cleanup( &ctx );
+ /*
+ * FIXME: We ignore the start value fd2 for the file descriptor search. This
+ * is not POSIX conform.
+ */
FIXME :) How to make it conform?
+++ 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()?
+++ b/cpukit/libcsupport/src/ftruncate.c
+ rv = (*iop->pathinfo.handlers->ftruncate_h)( iop, length );
why not locking this operation?
+++ b/cpukit/libcsupport/src/libio_sockets.c
+#include <rtems/rtems_bsdnet_internal.h>
why including this?
+++ b/cpukit/libcsupport/src/lseek.c
+ new_offset = reference_offset + offset;
Something wrong here. reference_offset undefined if default case hits.
+ rv = (*iop->pathinfo.handlers->lseek_h)( iop, offset, whence );
why not locking this operation?
+++ b/cpukit/libcsupport/src/mount.c
@@ -92,6 +81,23 @@ static rtems_filesystem_mount_table_entry_t
*alloc_mount_table_entry(
+ rtems_chain_initialize(
+ &mt_entry->location_chain,
+ mt_fs_root,
+ 1,
+ sizeof(*mt_fs_root)
+ );
+ } else {
+ free( mt_entry );
+ }
This is probably wrong since mt_entry would be NULL here.
Should set errno=ENOMEM
+static int register_subordinate_file_system(
+ rtems_filesystem_mount_table_entry_t *mt_entry,
+ const char *target
should be two leading spaces.
+int mount(
...
+ (*mt_entry->mt_fs_root->location.ops->fsunmount_me_h)( mt_entry );
why not locking this operation?
+ if ( rv != 0 ) {
+ free( mt_entry );
+ }
set errno?
+++ b/cpukit/libcsupport/src/open.c
+static int do_open(
...
+ rv = (*iop->pathinfo.handlers->open_h)( iop, path, oflag, mode );
why not locking this operation?
+ if ( rv != 0 ) {
+ (*iop->pathinfo.handlers->close_h)( iop );
+ }
why not locking this operation?
+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
+++ b/cpukit/libcsupport/src/sup_fs_location.c
+void rtems_filesystem_do_unmount(
+ (*mt_entry->mt_fs_root->location.ops->fsunmount_me_h)(mt_entry);
why not locking this operation?
+++ b/cpukit/libcsupport/src/unmount.c
+int unmount( const char *path )
...
+ rv = (*mt_entry->mt_point_node->location.ops->unmount_h)( mt_entry );
+ if ( rv == 0 ) {
+ rtems_filesystem_mt_entry_lock_context( lock_context );
+
+ rtems_filesystem_mt_entry_lock( lock_context );
+ mt_entry->mounted = false;
+ rtems_filesystem_mt_entry_unlock( lock_context );
+ }
why lock here? isn't this protected by eval_path_start instance lock?
+++ b/cpukit/libfs/src/devfs/devfs.h
/**
+ * The following defines the device-only filesystem operating
+ * handlers.
*/
imprecise comment i think these are the
device-only filesystem file operating handlers.
+extern const rtems_filesystem_file_handlers_r devFS_file_handlers;
+++ b/cpukit/libfs/src/imfs/imfs_rename.c
+ /*
+ * FIXME: Due to insufficient checks we can create inaccessible nodes with
+ * this operation.
+ */
FIXME :) Please explain a little more.
+++ b/cpukit/libfs/src/imfs/ioman.c
rtems_status_code rtems_io_lookup_name(
const char *name,
rtems_driver_name_t *device_info
)
now that this is not dependent on IMFS(?) should it be moved
+++ b/cpukit/libfs/src/nfsclient/src/nfs.c
while making these changes maybe fix the indent level of nfs functions?
A lot of these nfs functions are not indented according to our style guide
@@ -2035,7 +1799,8 @@ LOCK(nfsGlob.llock);
+ rtems_fatal_error_occurred(0xdeadbeef);
+ return;
unreached return? fix indent level.
@@ -3096,7 +2730,7 @@ Nfs nfs;
+ if (rtems_filesystem_resolve_location(mntpt, MAXPATHLEN,
&nfs->mt_entry->mt_fs_root->location))
line length >80
+++ b/cpukit/libfs/src/rfs/rtems-rfs-rtems.c
@@ -707,73 +400,46 @@
rtems_rfs_rtems_utime(rtems_filesystem_location_info_t* pathloc,
/**
* The following rouine creates a new symbolic link node under parent with the
+ * name given in name.
fix (preexisting) typo s/rouine/routine/.
Also change s/name given in name/name given in node_name/.
@@ -1304,19 +927,17 @@ rtems_rfs_rtems_initialise
(rtems_filesystem_mount_table_entry_t* mt_entry,
rtems_rfs_rtems_shutdown (rtems_filesystem_mount_table_entry_t* mt_entry)
+ /* FIXME: Return value? */
+ rtems_rfs_fs_close(fs);
FIXME :) Just eliminate comment, since rfs_fs_close appears always
to return 0.
+++ b/cpukit/libmisc/shell/main_lsof.c
+ printk(
+ "\t0x%08x -> 0x%08x\n",
+ loc,
+ loc->node_access
+ );
just put on one line?
+++ b/cpukit/libnetworking/lib/ftpfs.c
+static void rtems_ftpfs_eval_path(
+ rtems_filesystem_eval_path_context_t *self
)
Why doesn't this use the rtems_filesystem_eval_path_generic code?
+++ b/cpukit/libnetworking/lib/tftpDriver.c
+ fs = malloc (sizeof (*fs));
+ root_path = malloc (devicelen + 2);
+ if (root_path == NULL || fs == NULL)
+ goto error;
should just rtems_set_errno_and_return_minus_one (ENOMEM).
+
+error:
+
+ free (fs);
+ free (root_path);
+
+ rtems_set_errno_and_return_minus_one (ENOMEM);
this is wrong if jumping here because fs==NULL or root_path==NULL
+static void rtems_tftp_eval_path(rtems_filesystem_eval_path_context_t *self)
Why doesn't this use the rtems_filesystem_eval_path_generic code?
+ if (
+ eval_flags == 0
+ || eval_flags == RTEMS_LIBIO_PERMS_READ
+ || eval_flags == RTEMS_LIBIO_PERMS_WRITE
+ ) {
as with tfs is this equivalent to (eval_flags != RDW)?
+ memcpy(
+ current + currentlen,
+ path,
+ pathlen
+ );
put on one line?
More information about the devel
mailing list