RTEMS | libfs/fatfs: Add FatFS (!694)
Chris Johns (@chris)
gitlab at rtems.org
Thu Sep 11 03:07:45 UTC 2025
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694 was reviewed by Chris Johns
--
Chris Johns started a new discussion on cpukit/include/rtems/fatfs.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130726
> +
> +/*
> + Copyright (C) 2025 Embedded Brains GmbH
This is not EB's work but yours. Why have you left the copyright here?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/ffconf.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130727
>
> -#define FF_USE_MKFS 0
> +#define FF_USE_MKFS 1
Where does this file come from?
If this is an upstream file could they be defined on the compiler command line overriding them so we do not need to modify the upstream code?
Why do we not have `FF_FS_EXFAT` enabled?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130728
> +
> +/*
> + Copyright (C) 2025 Embedded Brains GmbH
Copyright again.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130729
> +static fatfs_volume_t volumes[ FF_VOLUMES ];
> +
> +DSTATUS disk_status( BYTE pdrv )
These calls need `rtems_fatfs_` to make sure they do not clash with user code.
If this is internal please add `static` here. I am not what `DSTATUS` is and if it contains `static`.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130730
> +}
> +
> +DSTATUS disk_initialize( BYTE pdrv )
Namespace `rtems_fatfs_` or `static`
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130731
> +}
> +
> +DRESULT disk_read( BYTE pdrv, BYTE *buff, LBA_t sector, UINT count )
Namespace `rtems_fatfs_`or `static`
What is the size or type of `LBA_t`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130732
> +}
> +
> +DRESULT disk_write( BYTE pdrv, const BYTE *buff, LBA_t sector, UINT count )
Namespace `rtems_fatfs_`or `static`
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130733
> +}
> +
> +DRESULT disk_ioctl( BYTE pdrv, BYTE cmd, void *buff )
Namespace `rtems_fatfs_`or `static`
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130734
> +#include "ff.h"
> +#include "diskio.h"
> +#include "rtems-fatfs.h"
Please group these to be after the standard headers. It is a common practice I do to make sure nothing in these local headers effect anything in the standard headers. ie.:
```c
#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
#include <rtems.h>
#include <rtems/bdbuf.h>
#include <rtems/libio_.h>
#include "ff.h"
#include "diskio.h"
#include "rtems-fatfs.h"
```
I also separate the RTEMS header out into a separate area.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130735
> + uint32_t block_size;
> +
> + if ( pdrv >= FF_VOLUMES || !volumes[ pdrv ].initialized ) {
Why not call `disk_status()`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130736
> + block_size = dd->block_size;
> +
> + for ( UINT i = 0; i < count; i++ ) {
Lets leave `UINT` and other internal types for FatFS. Please use `size_t`.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130737
> + uint32_t block_size;
> +
> + if ( pdrv >= FF_VOLUMES || !volumes[ pdrv ].initialized ) {
Why not call `disk_status()`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130738
> + block_size = dd->block_size;
> +
> + for ( UINT i = 0; i < count; i++ ) {
Please use `size_t`
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130739
> +DRESULT disk_ioctl( BYTE pdrv, BYTE cmd, void *buff )
> +{
> + if ( pdrv >= FF_VOLUMES || !volumes[ pdrv ].initialized ) {
Why not call `disk_status()`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130740
> + fd = open( device_path, O_RDWR );
> + if ( fd < 0 ) {
> + rtems_set_errno_and_return_minus_one( ENOENT );
Why override the error code from open? It could be a permissions problem?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-diskio.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130741
> + if ( rc != 0 ) {
> + close( fd );
> + rtems_set_errno_and_return_minus_one( EIO );
Same here.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-dir.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130742
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
Separate headers same as `rtems-diskio.c`
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-dir.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130743
> + fs_info->mount_path,
> + relative_path
> + );
This seems like an expensive way to concatenate two strings?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-dir.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130744
> + fs_info = (rtems_fatfs_fs_info_t *) iop->pathinfo.mt_entry->fs_info;
> +
> + rtems_fatfs_lock( iop->pathinfo.mt_entry );
Should the lock be held before you get `fs_info`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-dir.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130745
> +
> + if ( !node->is_open ) {
> + int rc = rtems_fatfs_get_full_path(
Why would we end up in a directory read call without the node being open?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-file.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130746
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
Separate header as stated before.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-file.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130747
> + rtems_set_errno_and_return_minus_one( ENAMETOOLONG );
> + }
> + return 0;
Is this is a common call maybe make a utility call to handle this. It will make maintaining the code easier as I have said using `snprintf` to concatenate two string is expensive and how you have to make the change twice (or more, I do not know).
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-file.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130748
> + }
> + node->is_open = true;
> + }
Is this code being repeated? A utility function to do this?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-file.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130749
> + }
> + node->is_open = true;
> + }
Same again.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-file.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130750
> + return -1;
> + }
> + node->is_open = true;
And again...
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130751
> +#include <sys/statvfs.h>
> +#include <time.h>
> +#include <unistd.h>
Separate header into groups as before.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130752
> + );
> + } else {
> + ret = snprintf(
Would 3 `strncat` calls be faster?
This looks great however in an kernel we want speed and `snprintf` has code to parse the format string a character at a time and we know all we want is to join the strings.
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs-init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130753
> +}
> +
> +DWORD get_fattime( void )
Internal call? Namespace or `static`?
--
Chris Johns started a new discussion on cpukit/libfs/src/fatfs/rtems-fatfs.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694#note_130754
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <time.h>
Grouping. See other comments
--
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/694
You're receiving this email because of your account on gitlab.rtems.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20250911/33151ebf/attachment-0001.htm>
More information about the bugs
mailing list