Re: FAT FS enhancements

[Andrew Lunn <andrew at lunn dot ch>]

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