Skip to content

FAT Filesystem error code maps to incorrect errors #6072

New issue

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

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

Already on GitHub? Sign in to your account

Closed
kegilbert opened this issue Feb 12, 2018 · 5 comments · Fixed by #6120 or #6336
Closed

FAT Filesystem error code maps to incorrect errors #6072

kegilbert opened this issue Feb 12, 2018 · 5 comments · Fixed by #6120 or #6336
Assignees

Comments

@kegilbert
Copy link
Contributor

The fat error remap case structure in the FATFileSystem class maps FR_INT_ERR, FR_MKFS_ABORTED, FR_TIMEOUT, and the default case to the a bad file error code which doesn't make much sense for the either a default or for the above errors.

Initially saw this during CI testing when I would receive the bad file error on format operations.

https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/fat/FATFileSystem.cpp#L66

@geky
Copy link
Contributor

geky commented Feb 15, 2018

@0xc0170, quick question, do we consider changing error codes to match documentation acceptable for patch releases? My gut says no, but at the same time these may be considered bugs?

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 16, 2018

@0xc0170, quick question, do we consider changing error codes to match documentation acceptable for patch releases? My gut says no, but at the same time these may be considered bugs?

If API specifies error codes and those are correct, it is a bug in the implementation. Should be fine for patch release (assuming it has clear commit msg and gets to the release notes).

If in doubt, we are close to feature release so can be fixed there

@kegilbert
Copy link
Contributor Author

Additionally should https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/fat/FATFileSystem.cpp#L64 be EINVAL rather than ENOEXEC?

@geky
Copy link
Contributor

geky commented Feb 16, 2018

Looking at this enum, here's what I'm thinking for the mapping:
EDIT: snipped table, see #6120 for up to date mapping

@geky
Copy link
Contributor

geky commented Feb 16, 2018

Respective pr: #6120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants