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

littlefs: Map LFS_ERR_CORRUPT to EILSEQ #6772

Merged
merged 1 commit into from May 23, 2018

Conversation

@geky
Member

geky commented Apr 29, 2018

Description

Previously EBAD (invalid exchange), mapping the error CORRUPT to EILSEQ (illegal byte sequence) makes more sense as a description of the type of error.

Will change the underlying error code inside littlefs on the next major version.

related #6336
cc @davidsaada, @kjbracey-arm, @deepikabhavnani

TODO

  • will squash

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[✗] Breaking change
@geky

This comment has been minimized.

Member

geky commented Apr 29, 2018

Added "do not merge" until removed from #6336 as they should go in together. I only want one release with error code changes.

@geky geky closed this Apr 29, 2018

@geky

This comment has been minimized.

Member

geky commented Apr 29, 2018

woops wrong button

@geky geky reopened this Apr 29, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 30, 2018

Little bit doubtful about EILSEQ because it is a quite new error originally defined by later C standards for multibyte text encodings. Not sure what the more traditional alternative would be, so probably fine.

@kegilbert

I'm fine with EILSEQ for a corruption error code. LGTM after squashing the extra commit.

@geky

This comment has been minimized.

Member

geky commented May 1, 2018

@kjbracey-arm I wanted to run this by you because I don't know a better errno code and have been looking for the past year or so. EBADE is used for SSL connection failures, but as far as static storage goes any sort of corruption detection in the OSs I've looked at are just returning as EIO.

But in our case, differentiating between a bus/driver issue and a corruption issue is important. Especially looking at security with respect to storage. I'm open to thoughts.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented May 2, 2018

@geky I earlier had a bit of search, trying to find out what mount/directory reads etc would traditionally do in Unices if presented with duff data, and like you didn't find any obvious answer.

Looking again at some old 4.4BSD source I have handy, I can see ISO CD mounting returning EINVAL, but UFS and similar seem to just use EIO. So they're either confused with device errors or user errors.

Hmph. Looks like something of a POSIX and predecessors omission.

Spreading a bit wider than POSIX, QNX has EBADFSYS and AIX has ECORRUPT.

I'd be kind of tempted to add add EBADFSYS.

@kjbracey-arm

Approving cos I don't have a clearly better suggestion.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 8, 2018

Spreading a bit wider than POSIX, QNX has EBADFSYS and AIX has ECORRUPT.
I would prefer one of these over EILSEQ, but don't feel strongly enough about it one way or another.

All three options are definitely a bit better than EBAD.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 14, 2018

@geky Do you still need to squash before we start CI?

littlefs: Map LFS_ERR_CORRUPT to EILSEQ
Previously EBAD (invalid exchange), mapping the error CORRUPT to EILSEQ
(illegal byte sequence) makes more sense as a description of the type of
error.

@geky geky dismissed stale reviews from kjbracey-arm and deepikabhavnani via 2697ebe May 14, 2018

@geky geky force-pushed the g-littlefs-eilseq branch from d9b2045 to 2697ebe May 14, 2018

@geky

This comment has been minimized.

Member

geky commented May 14, 2018

Done and done. Barring and major complaints, I think we should go ahead with EILSEQ.

@cmonr cmonr removed the do not merge label May 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 14, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels May 14, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented May 14, 2018

Build : SUCCESS

Build number : 2014
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6772/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels May 15, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 17, 2018

Holding off on merging until 5.8.5 is in.
Will bring in around same time that #6336 is brought in.

needs #6336

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 23, 2018

Build : SUCCESS

Build number : 2111
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6772/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@geky

This comment has been minimized.

Member

geky commented May 23, 2018

@cmonr LGTM?

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 23, 2018

Ooh! It's done!

@cmonr cmonr added ready for merge and removed needs: CI labels May 23, 2018

@cmonr

cmonr approved these changes May 23, 2018

👍

@cmonr cmonr merged commit eb9435b into master May 23, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
travis-ci/astyle Passed, 845 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10269 cycles (+1219 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 23, 2018

@geky

This comment has been minimized.

Member

geky commented May 23, 2018

@geky geky deleted the g-littlefs-eilseq branch May 30, 2018

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