[PATCH] Reference counting for file system locations

[Gedare Bloom]

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:
> Hello Gedare,
>
> thanks for the review.  I changed the things like this:
>
> http://git.rtems.org/sebh/rtems.git/commit/?h=fsrefcount&id=a78e2e075bcd28f91c82c99c8ca5b8a1e73c4cc1
>
>
> On 02/27/2012 10:28 PM, Gedare Bloom wrote:
>>
>> 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?
>>
>
> Both read and write is an error since we don't support seek?  It is the
> existing behaviour.  My intention was not to add more features.  The patch
> is large enough as it is.  I tried to only convert to the new API, nothing
> more.
>
>
>>
>> @@ -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.
>
>
> Yes, one more bug (the TFS and FTPFS was completely broken before the
> changes).  I fix this one.
>
>
>>
>>
>>
>> +++ 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?
>
>
> This saves one pointer and we don't need constant time extract.
>
>
>>
>>
>> +++ 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.
>
>
> Fixed.
>
>
>>
>>
>> + * @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./
>
>
> Fixed.
>
>
>>
>>
>> + * @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
>
>
> Fixed.
>
>
>>
>>
>> +/**
>> + * @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?
>
>
> Fixed.
>
>
>>
>>
>> -#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?
>
>
> Yes, this is a bit ugly.  I intend to add static asserts to ensure the the
> S_* and the path evaluation flags are compatible.  Maybe we should rename
> these defines in a follow up patch.
>
>
>>
>>
>> +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?
>
>
> See mount().
>
>
>>
>>
>> +++ 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?
>
>
> I have now
>
> rtems_filesystem_clonenode_t clonenod_h;
> rtems_filesystem_freenode_t freenod_h;
>
>> Maybe rename: filesystem_location_clone().
>
>
> I wanted to stay in line with rtems_filesystem_freenode().
>
>
>>
>>
>> +#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.

>
>>
>>
>> +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.
>
>
> Currently we have only handlers, but this may change in the future.
>
>
>>
>>
>> +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.
>
>
> Changed it to
>
> rtems_filesystem_location_copy()
>
>
>>
>>
>> +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.
>
>
> Please grep for 'static inline' in the cpukit.
>
Fair enough. I remember hearing complaints about the use of inline,
but unless someone goes through and changes them all then I don't care
if we add more.

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

>
>> 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?
>
>
> No, this is a coincidence.
>
>
>>
>>
>> +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?
>
>
> No, we assign it to a non-const pointer.
>
>
>>
>>
>> +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.
>
>
> Changed it to
>
> rtems_filesystem_location_detach()
> rtems_filesystem_location_copy_and_detach()
>
>>
>>
>> +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.
>
>
> Fixed.
>
>
>>
>>
>> +static inline bool rtems_filesystem_is_root_node(
>> +  const rtems_filesystem_location_info_t *loc
>> +)
>> I suggest filesysytem_location_is_root?
>
>
> Fixed.
>
>
>>
>>
>> +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.
>
>
> Fixed.
>
>
>>
>> +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?
>
>
> Fixed.
>
>
>>
>>
>> +static inline bool rtems_filesystem_is_directory_up(
>> +  const char *token,
>> +  size_t tokenlen
>> +)
>> maybe rtems_filesystem_is_parent_directory()?
>
>
> Not sure.
>
>
>>
>>
>> +++ 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.

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

>
>>
>>
>> +++ 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?
>
>
> Please look here:
>
> http://pubs.opengroup.org/onlinepubs/007904875/functions/rename.html
>
> Currently the rename is completely broken (also lseek and truncate).
>
>
>>
>>
>> +++ 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?
>
>
> No, it is set in rtems_filesystem_global_location_transform().
>
>
>>
>>
>> +++ 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?
>
>
> No, this is a feature.  Otherwise you are trapped in a chroot environment if
> someone unmounts the file system, e.g. someone uses an USB stick and does a
> chroot into a fs of this stick.
>
>
>>
>>
>> +      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 );
>
>
> The environment functions are a bit special since they sometimes set the
> errno.
>
>
>>
>>
>> +  /*
>> +   * FIXME: We ignore the start value fd2 for the file descriptor search.
>>  This
>> +   * is not POSIX conform.
>> +   */
>> FIXME :) How to make it conform?
>
>
> This is up to another patch.  No new features in this patch.
>
>
>>
>>
>> +++ 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.

>
>>
>>
>> +++ b/cpukit/libcsupport/src/ftruncate.c
>>
>> +    rv = (*iop->pathinfo.handlers->ftruncate_h)( iop, length );
>> why not locking this operation?
>
>
> The lock currently only covers the file system operations not the node
> handlers.  This is another issue.  See
>
> https://www.rtems.org/bugzilla/show_bug.cgi?id=1255
>
Ah, OK great.

>
>>
>>
>> +++ b/cpukit/libcsupport/src/libio_sockets.c
>>
>> +#include<rtems/rtems_bsdnet_internal.h>
>> why including this?
>
>
> Remove it and look for warnings.
>
>
>>
>>
>> +++ b/cpukit/libcsupport/src/lseek.c
>>
>> +  new_offset = reference_offset + offset;
>> Something wrong here. reference_offset undefined if default case hits.
>
>
> Sorry, I fixed this already, but there is more broken than that, see
>
> https://www.rtems.org/bugzilla/show_bug.cgi?id=2027
>
>
>>
>>
>> +      rv = (*iop->pathinfo.handlers->lseek_h)( iop, offset, whence );
>> why not locking this operation?
>
>
> See above.
>
>>
>>
>> +++ 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
>
>
> Sorry, I changed this in the meantime.
>
>
>>
>>
>> +static int register_subordinate_file_system(
>> + rtems_filesystem_mount_table_entry_t *mt_entry,
>> + const char *target
>> should be two leading spaces.
>
>
> Fixed.
>
>
>>
>>
>> +int mount(
>> ...
>> +            (*mt_entry->mt_fs_root->location.ops->fsunmount_me_h)(
>> mt_entry );
>> why not locking this operation?
>
>
> The mount() and unmount () of on file system instance must be done by at
> most one thread at a time.
>
>
>>
>>
>> +        if ( rv != 0 ) {
>> +          free( mt_entry );
>> +        }
>> set errno?
>
>
> Set elsewhere.
>
>
>>
>>
>> +++ 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?
>
>
> See above.
>
>
>>
>>
>>
>> +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.

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?

>
>>
>>
>> +++ 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?
>
>
> See above.
>
>
>>
>>
>> +++ 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?
>
>
> The lock could be a real lock and not simply an interrupt disable/enable.  I
> want to make sure that the state is consistent within the critical section.
>
>
>>
>>
>> +++ 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;
>
>
> Fixed.
>
>
>>
>>
>> +++ 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.
>
>
> You can rename a parent into a subdirectoy of its child.
>
>
>>
>>
>> +++ 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
>
>
> Not sure.
>
>
>>
>> +++ 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
>
>
> Yes, the format is special.  I try (its hard) to keep the current style of a
> file.
>
>
>>
>>
>> @@ -2035,7 +1799,8 @@ LOCK(nfsGlob.llock);
>> +                rtems_fatal_error_occurred(0xdeadbeef);
>> +                return;
>> unreached return? fix indent level.
>
>
> Not in this patch.
>
What about the unreached return?

>
>>
>>
>> @@ -3096,7 +2730,7 @@ Nfs               nfs;
>> +               if (rtems_filesystem_resolve_location(mntpt, MAXPATHLEN,
>> &nfs->mt_entry->mt_fs_root->location))
>> line length>80
>
>
> Not in this patch (there are so many lines > 80 in this file).
>
>
>>
>> +++ 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/.
>
>
> Fixed.
>
>
>>
>>
>> @@ -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.
>
>
> And in the future?  We may change it to void, add an assert here or use a
> fatal error.
>
>
>>
>>
>> +++ b/cpukit/libmisc/shell/main_lsof.c
>>
>> +      printk(
>> +        "\t0x%08x ->  0x%08x\n",
>> +        loc,
>> +        loc->node_access
>> +      );
>> just put on one line?
>
>
> This is work in progress.
>
>
>>
>>
>> +++ 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?
>
>
> No new features in this patch.
>
>
>>
>>
>> +++ 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
>
>
> free(NULL) is a non-operation.
>
I keep forgetting that.

>
>>
>>
>> +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?
>
>
> No new features in this patch.
>
>
>>
>> +        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)?
>
>
> Fixed.
>
>
>>
>>
>> +                memcpy(
>> +                    current + currentlen,
>> +                    path,
>> +                    pathlen
>> +                );
>> put on one line?
>
>
> Fixed.
>
> Thanks a lot Gedare.
>
It was...fun. At least I got to learn a lot. :)

>
> --
> 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.