pkg/fatfs: Refactor path handling, minor fixes by jnohlgard · Pull Request #8615 · RIOT-OS/RIOT

jnohlgard · 2018-02-22T07:41:42Z

Contribution description

Move multiple identical/similar calls to snprintf to a separate internal function _build_abs_path for better maintainability
Remove stale defines FATFS_DIR_SIZE, FATFS_FILE_SIZE
Increase FATFS_MAX_VOL_STR_LEN a tiny bit to avoid any possible overflows, fixes a compilation warning/error in GCC 7 about a truncated format string (-Wformat-truncation)
Minor editorial changes in Doxygen
Word wrap README files for readability in terminal editors

Issues/PRs references

depends on #8614
contains fix for #8265

MichelRottleuthner

Increase FATFS_MAX_VOL_STR_LEN a tiny bit to avoid any possible overflows, fixes a compilation warning/error in GCC 7 about a truncated format string (-Wformat-truncation)

somehow I culdn't reproduce that (using gcc 7.3) and I don't see how 4 is too short for a string like '9:/\0'

MichelRottleuthner · 2018-02-26T11:03:18Z

sys/include/fs/fatfs.h


 /**
- * info of a single opened file
+ * @brief info of a single opened file


while at it, this could be changed to "FatFs file instance descriptor" for consistency with the above

jnohlgard · 2018-02-26T12:44:04Z

@MichelRottleuthner

somehow I culdn't reproduce that (using gcc 7.3) and I don't see how 4 is too short for a string like '9:/\0'

No, but "10:/\0" is too long.
If you are testing you need to be aware that the path refactoring commit hides the warning by removing the small stack allocated buffer in _mount and _umount

MichelRottleuthner · 2018-02-26T14:22:26Z

No, but "10:/\0" is too long.

one could argue that its unlikely to have >9 fat volumes on a riot device, but who knows... But even then, why 6 and not 5?

If you are testing you need to be aware that the path refactoring commit hides the warning by removing the small stack allocated buffer in _mount and _umount

With current master I only get the -Wformat-truncation error when reducing FATFS_MAX_VOL_STR_LEN to 3 (while 4 works just fine for me). Thats why I wondered if it's needed at all.

jnohlgard · 2018-02-27T14:53:42Z

I see this with BOARD=mulle and gcc 7.3.0:

"make" -C /home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs
In file included from /usr/arm-none-eabi/include/stdio.h:800:0,
                 from /home/jgn/work/src/riot/core/include/debug.h:27,
                 from /home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:34:
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c: In function ‘_umount’:
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:73:46: error: ‘:/’ directive output may be truncated writing 2 bytes into a region of size between 1 and 3 [-Werror=format-truncation=]
     snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
                                              ^
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:73:5: note: ‘__builtin_snprintf’ output between 4 and 6 bytes into a destination of size 4
     snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
     ^
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c: In function ‘_mount’:
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:49:46: error: ‘:/’ directive output may be truncated writing 2 bytes into a region of size between 1 and 3 [-Werror=format-truncation=]
     snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
                                              ^
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:49:5: note: ‘__builtin_snprintf’ output between 4 and 6 bytes into a destination of size 4
     snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
     ^
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c: In function ‘_fstat’:
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:254:67: error: ‘__builtin_snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
     snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s",
                                                                   ^
/home/jgn/work/src/riot/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:254:5: note: ‘__builtin_snprintf’ output between 4 and 37 bytes into a destination of size 36
     snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s",
     ^
cc1: all warnings being treated as errors
make[2]: *** [/home/jgn/work/src/riot/Makefile.base:81: /home/jgn/work/src/riot/tests/pkg_fatfs_vfs/bin/mulle/fatfs_vfs/fatfs_vfs.o] Error 1

Installed toolchain versions
----------------------------
          native gcc: gcc (Gentoo 7.3.0 p1.0) 7.3.0
          msp430-gcc: missing/error
             avr-gcc: avr-gcc (Gentoo 7.3.0 p1.0) 7.3.0
   arm-none-eabi-gcc: arm-none-eabi-gcc (Gentoo 7.3.0 p1.0) 7.3.0
    mips-mti-elf-gcc: mips-mti-elf-gcc (Codescape GNU Tools 2016.05-03 for MIPS MTI Bare Metal) 4.9.2
               clang: clang version 5.0.1 (tags/RELEASE_501/final)
arm-none-eabi-newlib: "2.5.0"
 mips-mti-elf-newlib: "2.1.0"
            avr-libc: "2.0.0" ("20150208")
            cppcheck: Cppcheck 1.81
          coccinelle: spatch version 1.0.6 compiled with OCaml version 4.05.0
                 git: git version 2.16.2

MichelRottleuthner · 2018-02-28T08:53:19Z

the only relevant diff to my setup seems to be the newlib version:

Installed toolchain versions
----------------------------
          native gcc: gcc (GCC) 7.3.0
          msp430-gcc: msp430-gcc (mspgcc_20120406) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
             avr-gcc: avr-gcc (GCC) 7.3.0
   arm-none-eabi-gcc: arm-none-eabi-gcc (Arch Repository) 7.3.0
    mips-mti-elf-gcc: mips-mti-elf-gcc (Codescape GNU Tools 2016.05-03 for MIPS MTI Bare Metal) 4.9.2
               clang: clang version 5.0.1 (tags/RELEASE_501/final)
arm-none-eabi-newlib: "3.0.0"
 mips-mti-elf-newlib: "2.1.0"
            avr-libc: "2.0.0" ("20150208")
            cppcheck: Cppcheck 1.82
          coccinelle: spatch version 1.0.6 compiled with OCaml version 4.05.0
                 git: git version 2.16.2

basilfx · 2018-03-14T10:11:44Z

#8614 is merged, so Waiting for other PR can be removed.

jnohlgard · 2018-03-14T10:56:31Z

@basilfx thanks

Prevents compile time warnings about truncation of the format string for fs_idx > 9

jnohlgard · 2018-03-14T11:00:04Z

Rebased, squashed minor editorial doxygen comment fix into the other doxygen commit.

cladmi

Looks ok, just some questions inline.

I did not tested for the moment.

cladmi · 2018-03-14T13:28:16Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c


-    char volume_str[FATFS_MAX_VOL_STR_LEN];
-    snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
+    _build_abs_path(fs_desc, "");


In this one, you replaced the volume_str by using fs->abs_path_str_buff is it ok to use this one ? Just asking I do not know whet it should be used for.

fs->abs_path_str_buff is used as a string buffer for building the absolute paths for the library calls in other parts of the VFS wrapper. Since it is already allocated inside the struct, I didn't see any point in adding another string buffer on the stack, even if it is only a few bytes long.

Ok, from the type definition, it looks like it should be used for this anyway.

cladmi · 2018-03-14T13:31:48Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c

+    _build_abs_path(fs_desc, from_path);

-    snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%d:/%s",
+    snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%u:/%s",


This format string is duplicated with _build_abs_path. Would it make sense to put the format string in a shared define to remove the magic value in the middle of the functions?

I guess it could be macroed, but I don't think it is necessary with only two occurrences. Before this PR, the format string was duplicated in all functions.

cladmi · 2018-03-14T13:44:44Z

Tested with native:

tests/pkg_fatfs_vfs everything has OK
tests/pkg_fatfs I can create a file and write to it, same way as in master.

cladmi

ACK, tested only on native.

cladmi · 2018-03-14T15:52:37Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c

+    _build_abs_path(fs_desc, from_path);

-    snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%d:/%s",
+    snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%u:/%s",


cladmi · 2018-03-14T15:53:06Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c


-    char volume_str[FATFS_MAX_VOL_STR_LEN];
-    snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx);
+    _build_abs_path(fs_desc, "");


Ok, from the type definition, it looks like it should be used for this anyway.

cladmi · 2018-03-14T15:55:40Z

I will test on mulle before merging.

cladmi · 2018-03-14T16:47:15Z

Hmm, mulle does not have any sdcard so cannot work…

@MichelRottleuthner do you have some hardware to test ?

MichelRottleuthner

No mulle here, but ran tests/pkg_fatfs_vfs and tests/pkg_fatfs with an SD-card on remote-revb both working fine -> another ACK from my side. Thanks for improving the code @gebart!

jnohlgard · 2018-03-15T08:21:16Z

Thanks for reviewing!

jnohlgard added this to the Release 2018.04 milestone Feb 22, 2018

jnohlgard requested a review from MichelRottleuthner February 22, 2018 07:41

jnohlgard mentioned this pull request Feb 22, 2018

Tracker: GCC 7.x compatibility #8265

Closed

13 tasks

MichelRottleuthner reviewed Feb 26, 2018

View reviewed changes

jnohlgard removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 14, 2018

Joakim Nohlgård added 5 commits March 14, 2018 11:56

pkg/fatfs: refactor absolute path handling

5fb07e8

pkg/fatfs: Remove unused FATFS_DIR_SIZE, FATFS_FILE_SIZE

0251f54

tests/pkg_fatfs_vfs: Word wrap README

f8a9e63

tests/pkg_fatfs: Word wrap README

0735614

fs/fatfs: Adjust FATFS_MAX_VOL_STR_LEN to avoid overflows

4ce675e

Prevents compile time warnings about truncation of the format string for fs_idx > 9

jnohlgard force-pushed the pr/fatfs-paths branch from 390a901 to 4f5961f Compare March 14, 2018 10:57

fs/fatfs: Editorial changes in Doxygen comments

0bc2c1b

jnohlgard force-pushed the pr/fatfs-paths branch from 4f5961f to 0bc2c1b Compare March 14, 2018 10:57

cladmi reviewed Mar 14, 2018

View reviewed changes

cladmi approved these changes Mar 14, 2018

View reviewed changes

MichelRottleuthner approved these changes Mar 14, 2018

View reviewed changes

jnohlgard merged commit 8873a71 into RIOT-OS:master Mar 15, 2018

jnohlgard deleted the pr/fatfs-paths branch March 15, 2018 08:21

Conversation

jnohlgard commented Feb 22, 2018

Contribution description

Issues/PRs references

Uh oh!

MichelRottleuthner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 26, 2018

Uh oh!

MichelRottleuthner commented Feb 26, 2018

Uh oh!

jnohlgard commented Feb 27, 2018

Uh oh!

MichelRottleuthner commented Feb 28, 2018

Uh oh!

basilfx commented Mar 14, 2018

Uh oh!

jnohlgard commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Mar 14, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

MichelRottleuthner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichelRottleuthner left a comment •

edited

Loading

jnohlgard commented Mar 14, 2018 •

edited

Loading

MichelRottleuthner left a comment •

edited

Loading