This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: FAT FS enhancements
On Wed, Oct 20, 2004 at 10:44:43PM -0700, David Brennan wrote:
> Let me know what names you want changed around, and if this is the
> proper structure for doing this type of change please.
I like this much better. I just have a few minor comments...
> +#ifdef CYGCFG_FS_FAT_USE_ATTRIBUTES
> + case FS_INFO_ATTRIB:
> + err = fatfs_get_attrib(mte, dir, name,
> (cyg_fatfs_attrib_t*)buf);
> + if (err != ENOERR)
> + return err;
> + break;
> +#endif // CYGCFG_FS_FAT_USE_ATTRIBUTES
> + default:
> + err = EINVAL;
> + break;
> + }
> + return err;
There is no need to check the return value from fatfs_get_attrib
here. You return err if you return imeadiately or via the end of the
function.
> + switch( key )
> + {
> + case FS_INFO_SYNC:
> + err = cyg_blib_sync(&(((fatfs_disk_t *) mte->data)->blib));
> + if (err != ENOERR)
> + return err;
> + break;
> +#ifdef CYGCFG_FS_FAT_USE_ATTRIBUTES
> + case FS_INFO_ATTRIB:
> + err = fatfs_set_attrib(mte, dir, name, *(cyg_fatfs_attrib_t
> *)buf);
> + if (err != ENOERR)
> + return err;
> + break;
> +#endif // CYGCFG_FS_FAT_USE_ATTRIBUTES
> + default:
> + err = EINVAL;
> + break;
> + }
> + return err;
Same comment as above
> +#ifdef CYGCFG_FS_FAT_USE_ATTRIBUTES
> + // Create file
> + diag_printf("<INFO>: create /foo\n");
> + createfile( "/foo", 20257 );
More readable is you use the symbolc constants OR'd together.
> +typedef cyg_uint32 cyg_fatfs_attrib_t;
[..]
> +// Functions to provide the equivilent of DOS ATTRIB function. Only
> applicable
> +// to FAT file systems at the moment.
> +__externC int cyg_set_attrib( const char *fname,
> + const cyg_fatfs_attrib_t new_attrib );
> +__externC int cyg_get_attrib( const char *fname,
> + cyg_fatfs_attrib_t * const file_attrib );
> +
These names are too generic/too specific.
Any sort of object could have attributes, but your
cyg_{set|get}_attrib() only apply to filesystems. So maybe its better
to call them cyg_fs_{set|get}_attrib(). But the parameter type
cyg_fatfs_attrib_t is specific to FATFS.
I think making this a generic cyg_fs_attrib_t is better, rather than
have functions cyg_fs_fatfs_{get|set}_attrib(). But i'm wondering if
that is generic enough? Maybe it should be a void * and a len so you
can pass anything?
> +__externC int cyg_fssync( const char *path )
cyg_fs_fssync() fits the pattern better. All other functions start
with cyg_fs_
I would put the following into a new file, <cyg/fs/fatfs.h>
> +//
> -------------------------------------------------------------------------
> +// FAT filesystem dir entry attributes
> +
> +#define S_FATFS_RDONLY (0x01) // Read only
> +#define S_FATFS_HIDDEN (0x02) // Hidden
> +#define S_FATFS_SYSTEM (0x04) // System
> +#define S_FATFS_VOLUME (0x08) // Volume label
> +#define S_FATFS_DIR (0x10) // Subdirectory
> +#define S_FATFS_ARCHIVE (0x20) // Needs archiving
> +
> +// Mode bits which are allowed by chmod
> +#define S_FATFS_ATTRIB (S_FATFS_RDONLY | S_FATFS_HIDDEN |
> S_FATFS_SYSTEM | \
> + S_FATFS_ARCHIVE)
> +//
> -------------------------------------------------------------------------
> +// mode FAT dir entry attributes macros
> +
> +#define MODE_IS_RDONLY(__mode) ((__mode) & S_FATFS_RDONLY)
> +#define MODE_IS_HIDDEN(__mode) ((__mode) & S_FATFS_HIDDEN)
> +#define MODE_IS_SYSTEM(__mode) ((__mode) & S_FATFS_SYSTEM)
> +#define MODE_IS_VOLUME(__mode) ((__mode) & S_FATFS_VOLUME)
> +#define MODE_IS_DIR(__mode) ((__mode) & S_FATFS_DIR)
> +#define MODE_IS_ARCHIVE(__mode) ((__mode) & S_FATFS_ARCHIVE)
The normal convention is
#define S_ISDIR(__mode) ((__mode) & __stat_mode_DIR )
so i would say
#define S_FATFS_ISRDONLY(__mode) ((__mode) & S_FATFS_RDONLY)
#define S_FATFS_ISHIDDEN(__mode) ((__mode) & S_FATFS_HIDDEN)
etc.
Andrew