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

Fix is_valid_erase function to use get_erase_size with address #7953

Merged
merged 1 commit into from Sep 27, 2018

Conversation

Projects
None yet
6 participants
@davidsaada
Contributor

davidsaada commented Sep 2, 2018

Description

The base BlockDevice class includes the is_valid_erase function checking whether the address and size given to the erase API are valid. This function used to call the get_erase_size function without the address argument, but this was a bug, as it would have failed valid calls to erase. It now calls get_erase_size with the address argument.
Just to clarify - this will work also for classes that don't implement get_erase_size with the address parameter, as it defaults to the one that doesn't have the address in the base class.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@NirSonnenschein NirSonnenschein requested a review from ARMmbed/mbed-os-storage Sep 2, 2018

addr % get_erase_size() == 0 &&
size % get_erase_size() == 0 &&
addr % get_erase_size(addr) == 0 &&
size % get_erase_size(addr) == 0 &&

This comment has been minimized.

@offirko

offirko Sep 3, 2018

Contributor

You check that 'size' is aligned with current 'addr' but a large 'size' might take you to a 'dest' address in a different sector with a different 'erase-size' than 'addr'.
Basically I think you need to verify that:
( (addr+size) % get_erase_size(addr+size-1) == 0 )

This comment has been minimized.

@davidsaada

davidsaada Sep 3, 2018

Contributor

You are right about my code, but your solution is also problematic, as it only checks one sector. The right solution would be to iterate on the remaining sectors. Will fix that.

This comment has been minimized.

@davidsaada

davidsaada Sep 3, 2018

Contributor

You are right about the problem in my code, but your solution probably won't work either (as both only cover one sector). Right solution is probably an iteration over the remaining of the sectors. I'll fix it.

This comment has been minimized.

@offirko

offirko Sep 3, 2018

Contributor

@davidsaada - My thinking is that if the 'start' delete address is aligned to its sector, and the 'end' delete address is aligned to its own sector - then every sector in between them will be fully erased - so there's no need to iterate. What do you think?

This comment has been minimized.

@davidsaada

davidsaada Sep 3, 2018

Contributor

You're right - better than iteration. Will fix.

This comment has been minimized.

@davidsaada

davidsaada Sep 3, 2018

Contributor

Fixed. Please review.

@davidsaada davidsaada force-pushed the davidsaada:david_is_valid_erase_fix branch 2 times, most recently from 49f14a4 to e1c0b85 Sep 3, 2018

@davidsaada davidsaada force-pushed the davidsaada:david_is_valid_erase_fix branch from e1c0b85 to ff7858a Sep 3, 2018

@offirko

offirko approved these changes Sep 3, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 26, 2018

Build : SUCCESS

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

Triggering tests

/morph 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 Sep 27, 2018

@cmonr cmonr merged commit 6b85ec7 into ARMmbed:master Sep 27, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(-0.09%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 597 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9927 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Sep 27, 2018

@davidsaada davidsaada deleted the davidsaada:david_is_valid_erase_fix branch Dec 6, 2018

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