Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

pkg/fatfs/fatfs_vfs: fix flag translation in _open #14175

Merged
merged 2 commits into from
Jun 29, 2020
Merged

pkg/fatfs/fatfs_vfs: fix flag translation in _open #14175

merged 2 commits into from
Jun 29, 2020

Conversation

sven-hm
Copy link
Contributor

@sven-hm sven-hm commented May 29, 2020 •
edited
Loading

Contribution description

When using fatfs_vfs with newlib, fopen will fail in append-mode ("a") if the file you want to open already exists.
Using vfs_open(..., O_WRONLY | O_CREAT, 0) on an existing file will also fail.

This PR extends the test_create-test case and provides a new test case for fopen etc.
It fixes the mode flag translation to FatFs's FA_-flags. The translation should be as in the table:

POSIX O_-flags FatFs FA_-flags
"r" O_RDONLY FA_READ
"r+" O_RDWR FA_READ,FA_WRITE
"w" O_WRONLY, O_CREAT, O_TRUNC FA_WRITE, FA_CREATE_ALWAYS
"w+" O_RDWR, O_CREAT, O_TRUNC FA_READ,FA_WRITE, FA_CREATE_ALWAYS
"a" O_WRONLY, O_CREAT, O_APPEND FA_OPEN_APPEND
"a+" O_RDWR, O_CREAT, O_APPEND FA_READ, FA_OPEN_APPEND
"wx" O_WRONLY, O_CREAT, O_TRUNC,O_EXCL FA_WRITE, FA_CREATE_NEW
"w+x" O_RDWR, O_CREAT, O_TRUNC,O_EXCL FA_READ,FA_WRITE, FA_CREATE_NEW
? O_WRONLY, O_CREAT FA_WRITE,FA_OPEN_ALWAYS

Compare flag-table here.

Testing procedure

Run these tests on your hardware with an SD-card as described in the REAME.

@benpicco benpicco added Area: fs Area: File systems Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 29, 2020
@benpicco benpicco requested a review from bergzand May 29, 2020 17:14
@benpicco
Copy link
Contributor

benpicco commented May 29, 2020 •
edited
Loading

I ran the new test on same54-xpro with an SD card attached.

before 
2020-05-29 19:19:33,954 # test_mount__mount:[OK]
2020-05-29 19:19:33,956 # test_mount__umount:[OK]
2020-05-29 19:19:33,963 # test_open__mount:[OK]
2020-05-29 19:19:33,967 # test_open__open:[OK]
2020-05-29 19:19:33,969 # test_open__open_ro:[OK]
2020-05-29 19:19:33,971 # test_open__close_ro:[OK]
2020-05-29 19:19:33,973 # test_open__open_wo:[OK]
2020-05-29 19:19:33,975 # test_open__close_wo:[OK]
2020-05-29 19:19:33,977 # test_open__open_rw:[OK]
2020-05-29 19:19:33,980 # test_open__close_rw:[OK]
2020-05-29 19:19:33,982 # test_open__umount:[OK]
2020-05-29 19:19:33,988 # test_rw__mount:[OK]
2020-05-29 19:19:33,992 # test_rw__open_ro:[OK]
2020-05-29 19:19:33,995 # test_rw__read_ro:[OK]
2020-05-29 19:19:33,997 # test_rw__write_ro:[OK]
2020-05-29 19:19:33,999 # test_rw__close_ro:[OK]
2020-05-29 19:19:34,003 # test_rw__open_wo:[OK]
2020-05-29 19:19:34,005 # test_rw__read_wo:[OK]
2020-05-29 19:19:34,007 # test_rw__close_wo:[OK]
2020-05-29 19:19:34,009 # test_rw__open_rw:[OK]
2020-05-29 19:19:34,012 # test_rw__read_rw:[OK]
2020-05-29 19:19:34,014 # test_rw__write_rw:[OK]
2020-05-29 19:19:34,016 # test_rw__lseek:[OK]
2020-05-29 19:19:34,018 # test_rw__read_rw:[OK]
2020-05-29 19:19:34,077 # test_rw__close_rw:[OK]
2020-05-29 19:19:34,079 # test_rw__open_rwc:[OK]
2020-05-29 19:19:34,093 # test_rw__write_rwc:[OK]
2020-05-29 19:19:34,096 # test_rw__lseek_rwc:[OK]
2020-05-29 19:19:34,098 # test_rw__read_rwc:[OK]
2020-05-29 19:19:34,112 # test_rw__close_rwc:[OK]
2020-05-29 19:19:34,114 # test_rw__umount:[OK]
2020-05-29 19:19:34,121 # test_dir__mount:[OK]
2020-05-29 19:19:34,123 # test_dir__opendir:[OK]
2020-05-29 19:19:34,127 # test_dir__readdir1:[OK]
2020-05-29 19:19:34,129 # test_dir__readdir2:[OK]
2020-05-29 19:19:34,131 # test_dir__readdir_name:[OK]
2020-05-29 19:19:34,134 # test_dir__readdir3:[FAILED]
2020-05-29 19:19:34,136 # test_dir__closedir:[OK]
2020-05-29 19:19:34,138 # test_dir__umount:[OK]
2020-05-29 19:19:34,144 # test_rename__mount:[OK]
2020-05-29 19:19:34,152 # test_rename__rename:[OK]
2020-05-29 19:19:34,154 # test_rename__opendir:[OK]
2020-05-29 19:19:34,157 # test_rename__readdir1:[OK]
2020-05-29 19:19:34,159 # test_rename__readdir2:[OK]
2020-05-29 19:19:34,162 # test_rename__check_name:[OK]
2020-05-29 19:19:34,164 # test_rename__readdir3:[FAILED]
2020-05-29 19:19:34,167 # test_rename__closedir:[OK]
2020-05-29 19:19:34,169 # test_rename__umount:[OK]
2020-05-29 19:19:34,176 # test_unlink__mount:[OK]
2020-05-29 19:19:34,196 # test_unlink__unlink1:[OK]
2020-05-29 19:19:34,216 # test_unlink__unlink2:[OK]
2020-05-29 19:19:34,218 # test_unlink__opendir:[OK]
2020-05-29 19:19:34,223 # test_unlink__readdir:[FAILED]
2020-05-29 19:19:34,225 # test_unlink__closedir:[OK]
2020-05-29 19:19:34,227 # test_unlink__umount:[OK]
2020-05-29 19:19:34,234 # test_mkrmdir__mount:[OK]
2020-05-29 19:19:34,363 # test_mkrmdir__mkdir:[OK]
2020-05-29 19:19:34,367 # test_mkrmdir__opendir1:[OK]
2020-05-29 19:19:34,370 # test_mkrmdir__closedir:[OK]
2020-05-29 19:19:34,388 # test_mkrmdir__rmdir:[OK]
2020-05-29 19:19:34,393 # test_mkrmdir__opendir2:[OK]
2020-05-29 19:19:34,395 # test_mkrmdir__umount:[OK]
2020-05-29 19:19:34,402 # test_create__mount:[OK]
2020-05-29 19:19:34,406 # test_create__open_wo:[OK]
2020-05-29 19:19:34,418 # test_create__write_wo:[OK]
2020-05-29 19:19:34,430 # test_create__close_wo:[OK]
2020-05-29 19:19:34,432 # test_create__umount:[OK]
2020-05-29 19:19:34,440 # test_newlib__mount:[OK]
2020-05-29 19:19:34,444 # test_newlib__fopen:[OK]
2020-05-29 19:19:34,446 # test_newlib__fopen_r:[OK]
2020-05-29 19:19:34,448 # test_newlib__fclose_r:[OK]
2020-05-29 19:19:34,451 # test_newlib__fopen_w:[OK]
2020-05-29 19:19:34,453 # test_newlib__fputs_w:[OK]
2020-05-29 19:19:34,465 # test_newlib__fread_w:[OK]
2020-05-29 19:19:34,468 # test_newlib__strcmp_w:[OK]
2020-05-29 19:19:34,530 # test_newlib__fclose_w:[OK]
2020-05-29 19:19:34,547 # test_newlib__remove:[OK]
2020-05-29 19:19:34,551 # test_newlib__fopen_a:[OK]
2020-05-29 19:19:34,554 # test_newlib__fputs_a:[OK]
2020-05-29 19:19:34,576 # test_newlib__fclose_a:[OK]
2020-05-29 19:19:34,581 # test_newlib__fopen_a2:[FAILED]
2020-05-29 19:19:34,583 # test_newlib__fputs_a2:[OK]
2020-05-29 19:19:34,583 # 
2020-05-29 19:19:34,585 # Context before hardfault:
2020-05-29 19:19:34,587 #    r0: 0x2000025c
2020-05-29 19:19:34,589 #    r1: 0x00000000
2020-05-29 19:19:34,590 #    r2: 0x00000611
2020-05-29 19:19:34,592 #    r3: 0x1ffffbef
2020-05-29 19:19:34,593 #   r12: 0x00000000
2020-05-29 19:19:34,595 #    lr: 0x00005819
2020-05-29 19:19:34,596 #    pc: 0x00000000
2020-05-29 19:19:34,598 #   psr: 0x200f0000
2020-05-29 19:19:34,598 # 
2020-05-29 19:19:34,599 # FSR/FAR:
2020-05-29 19:19:34,601 #  CFSR: 0x00020000
2020-05-29 19:19:34,602 #  HFSR: 0x40000000
2020-05-29 19:19:34,604 #  DFSR: 0x00000000
2020-05-29 19:19:34,605 #  AFSR: 0x00000000
2020-05-29 19:19:34,606 # Misc
2020-05-29 19:19:34,607 # EXC_RET: 0xfffffffd
2020-05-29 19:19:34,612 # Attempting to reconstruct state for debugging...
2020-05-29 19:19:34,612 # In GDB:
2020-05-29 19:19:34,614 #   set $pc=0x0
2020-05-29 19:19:34,614 #   frame 0
2020-05-29 19:19:34,615 #   bt
2020-05-29 19:19:34,615 # 
2020-05-29 19:19:34,619 # ISR stack overflowed by at least 16 bytes.
with this patch 
2020-05-29 19:20:04,308 # test_mount__mount:[OK]
2020-05-29 19:20:04,311 # test_mount__umount:[OK]
2020-05-29 19:20:04,317 # test_open__mount:[OK]
2020-05-29 19:20:04,321 # test_open__open:[OK]
2020-05-29 19:20:04,323 # test_open__open_ro:[OK]
2020-05-29 19:20:04,325 # test_open__close_ro:[OK]
2020-05-29 19:20:04,327 # test_open__open_wo:[OK]
2020-05-29 19:20:04,329 # test_open__close_wo:[OK]
2020-05-29 19:20:04,332 # test_open__open_rw:[OK]
2020-05-29 19:20:04,334 # test_open__close_rw:[OK]
2020-05-29 19:20:04,336 # test_open__umount:[OK]
2020-05-29 19:20:04,342 # test_rw__mount:[OK]
2020-05-29 19:20:04,346 # test_rw__open_ro:[OK]
2020-05-29 19:20:04,349 # test_rw__read_ro:[OK]
2020-05-29 19:20:04,351 # test_rw__write_ro:[OK]
2020-05-29 19:20:04,353 # test_rw__close_ro:[OK]
2020-05-29 19:20:04,357 # test_rw__open_wo:[OK]
2020-05-29 19:20:04,359 # test_rw__read_wo:[OK]
2020-05-29 19:20:04,361 # test_rw__close_wo:[OK]
2020-05-29 19:20:04,363 # test_rw__open_rw:[OK]
2020-05-29 19:20:04,366 # test_rw__read_rw:[OK]
2020-05-29 19:20:04,368 # test_rw__write_rw:[OK]
2020-05-29 19:20:04,370 # test_rw__lseek:[OK]
2020-05-29 19:20:04,372 # test_rw__read_rw:[OK]
2020-05-29 19:20:04,382 # test_rw__close_rw:[OK]
2020-05-29 19:20:04,383 # test_rw__open_rwc:[OK]
2020-05-29 19:20:04,388 # test_rw__write_rwc:[OK]
2020-05-29 19:20:04,390 # test_rw__lseek_rwc:[OK]
2020-05-29 19:20:04,392 # test_rw__read_rwc:[OK]
2020-05-29 19:20:04,401 # test_rw__close_rwc:[OK]
2020-05-29 19:20:04,403 # test_rw__umount:[OK]
2020-05-29 19:20:04,410 # test_dir__mount:[OK]
2020-05-29 19:20:04,412 # test_dir__opendir:[OK]
2020-05-29 19:20:04,416 # test_dir__readdir1:[OK]
2020-05-29 19:20:04,418 # test_dir__readdir2:[OK]
2020-05-29 19:20:04,421 # test_dir__readdir_name:[OK]
2020-05-29 19:20:04,423 # test_dir__readdir3:[FAILED]
2020-05-29 19:20:04,425 # test_dir__closedir:[OK]
2020-05-29 19:20:04,427 # test_dir__umount:[OK]
2020-05-29 19:20:04,434 # test_rename__mount:[OK]
2020-05-29 19:20:04,441 # test_rename__rename:[OK]
2020-05-29 19:20:04,443 # test_rename__opendir:[OK]
2020-05-29 19:20:04,445 # test_rename__readdir1:[OK]
2020-05-29 19:20:04,448 # test_rename__readdir2:[OK]
2020-05-29 19:20:04,450 # test_rename__check_name:[OK]
2020-05-29 19:20:04,453 # test_rename__readdir3:[FAILED]
2020-05-29 19:20:04,455 # test_rename__closedir:[OK]
2020-05-29 19:20:04,457 # test_rename__umount:[OK]
2020-05-29 19:20:04,465 # test_unlink__mount:[OK]
2020-05-29 19:20:04,481 # test_unlink__unlink1:[OK]
2020-05-29 19:20:04,499 # test_unlink__unlink2:[OK]
2020-05-29 19:20:04,501 # test_unlink__opendir:[OK]
2020-05-29 19:20:04,506 # test_unlink__readdir:[FAILED]
2020-05-29 19:20:04,508 # test_unlink__closedir:[OK]
2020-05-29 19:20:04,510 # test_unlink__umount:[OK]
2020-05-29 19:20:04,517 # test_mkrmdir__mount:[OK]
2020-05-29 19:20:04,629 # test_mkrmdir__mkdir:[OK]
2020-05-29 19:20:04,634 # test_mkrmdir__opendir1:[OK]
2020-05-29 19:20:04,636 # test_mkrmdir__closedir:[OK]
2020-05-29 19:20:04,654 # test_mkrmdir__rmdir:[OK]
2020-05-29 19:20:04,659 # test_mkrmdir__opendir2:[OK]
2020-05-29 19:20:04,661 # test_mkrmdir__umount:[OK]
2020-05-29 19:20:04,668 # test_create__mount:[OK]
2020-05-29 19:20:04,672 # test_create__open_wo:[FAILED]
2020-05-29 19:20:04,675 # test_create__write_wo:[FAILED]
2020-05-29 19:20:04,678 # test_create__close_wo:[FAILED]
2020-05-29 19:20:04,680 # test_create__umount:[OK]
2020-05-29 19:20:04,687 # test_newlib__mount:[OK]
2020-05-29 19:20:04,691 # test_newlib__fopen:[OK]
2020-05-29 19:20:04,693 # test_newlib__fopen_r:[FAILED]
2020-05-29 19:20:04,696 # test_newlib__fclose_r:[OK]
2020-05-29 19:20:04,698 # test_newlib__fopen_w:[OK]
2020-05-29 19:20:04,700 # test_newlib__fputs_w:[OK]
2020-05-29 19:20:04,762 # test_newlib__fread_w:[OK]
2020-05-29 19:20:04,764 # test_newlib__strcmp_w:[OK]
2020-05-29 19:20:04,776 # test_newlib__fclose_w:[OK]
2020-05-29 19:20:04,793 # test_newlib__remove:[OK]
2020-05-29 19:20:04,798 # test_newlib__fopen_a:[OK]
2020-05-29 19:20:04,800 # test_newlib__fputs_a:[OK]
2020-05-29 19:20:04,823 # test_newlib__fclose_a:[OK]
2020-05-29 19:20:04,827 # test_newlib__fopen_a2:[OK]
2020-05-29 19:20:04,829 # test_newlib__fputs_a2:[OK]
2020-05-29 19:20:04,833 # test_newlib__fread_a2:[OK]
2020-05-29 19:20:04,836 # test_newlib__strcmp_a2:[OK]
2020-05-29 19:20:04,838 # test_newlib__strcmp_a2:[OK]
2020-05-29 19:20:04,848 # test_newlib__fclose_a2:[OK]
2020-05-29 19:20:04,863 # test_newlib__remove:[OK]
2020-05-29 19:20:04,865 # test_newlib__umount:[OK]
2020-05-29 19:20:04,866 # Test end.

→ test_create fails now.

@sven-hm sven-hm force-pushed the fatfs_vfs_open_flag_translation branch from 9ee6680 to 1ba1133 Compare May 29, 2020 17:28
@benpicco benpicco removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label May 29, 2020
benpicco
benpicco reviewed Jun 2, 2020
View reviewed changes
@sven-hm sven-hm force-pushed the fatfs_vfs_open_flag_translation branch from dee5d69 to 1715c2e Compare June 2, 2020 08:42
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2020
benpicco
benpicco reviewed Jun 2, 2020
View reviewed changes
@sven-hm sven-hm force-pushed the fatfs_vfs_open_flag_translation branch 2 times, most recently from 5e2b858 to ea50270 Compare June 2, 2020 11:47
benpicco
benpicco approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, all tests (except the readdir3 ones, but they failed before) succeed now!

@sven-hm
Copy link
Contributor Author

sven-hm commented Jun 2, 2020 •
edited
Loading

vfs_open failed also with O_WRONLY | O_CREAT if the file already exists, i extended the test_create test case. I updated the PR-description above.

@sven-hm
Copy link
Contributor Author

sven-hm commented Jun 2, 2020

Looks good, all tests (except the readdir3 ones, but they failed before) succeed now!

readdir3 succeeds here (on master, too)

@benpicco
Copy link
Contributor

benpicco commented Jun 3, 2020

@kaspar030 do you know what's with that

tests/pkg_fatfs_vfs/main.c:335: error (resourceLeak): Resource leak: fl

I don't see a resource leak there - fl is always closed.
And I don't get that error when running ./dist/tools/cppcheck/check.sh locally.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 11, 2020
@kaspar030
Copy link
Contributor

I don't see a resource leak there - fl is always closed.

agreed.

And I don't get that error when running ./dist/tools/cppcheck/check.sh locally.

Same here, I'm on Cppcheck 2.0.

So let's assume a Cppcheck bug!

@kaspar030
Copy link
Contributor

hit by #14264.

@benpicco
Copy link
Contributor

So let's assume a Cppcheck bug!

Can we update cppcheck on CI?

@benpicco benpicco requested review from vincent-d and miri64 June 25, 2020 16:27
miri64
miri64 reviewed Jun 29, 2020
View reviewed changes
tests/pkg_fatfs_vfs/main.c Outdated
Comment on lines 334 to 335
/* open file RO */
fl = fopen(FULL_FNAME2, "r");
Copy link
Member

@miri64 miri64 Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let's assume a Cppcheck bug!

Can we update cppcheck on CI?

Would be easier to suppress this warning for now. @sven-hm please add cppcheck suppression comment:

Suggested change
/* open file RO */
fl = fopen(FULL_FNAME2, "r");
/* cppcheck-suppress resourceLeak
* (reason: cppcheck <2.0 reports a false positive here */
fl = fopen(FULL_FNAME2, "r"); /* open file RO */

sven-hm added 2 commits June 29, 2020 10:06
@sven-hm
tests/pkg_fatfs_vfs: add tests
05352b6 
* vfs_open with `O_WRONLY | O_CREAT` if file exists
* of fopen etc. with newlib
@sven-hm
pkg/fatfs/fatfs_vfs: fix flag translation in _open
4469d8f
@benpicco benpicco merged commit 3af5efe into RIOT-OS:master Jun 29, 2020
@miri64 miri64 added this to the Release 2020.07 milestone Jun 30, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@miri64 miri64 miri64 left review comments

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@bergzand bergzand Awaiting requested review from bergzand

@vincent-d vincent-d Awaiting requested review from vincent-d

Assignees
No one assigned
Labels
Area: fs Area: File systems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.
4 participants
@sven-hm @benpicco @kaspar030 @miri64