[PATCH] Reference counting for file system locations
Gedare Bloom
gedare at rtems.org
Tue Feb 28 16:49:09 UTC 2012
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.
More information about the devel
mailing list