Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MBRBlockDevice: When partitioning, clear the rest of first erase unit #9384

Merged
merged 1 commit into from
Jan 16, 2019
Merged

MBRBlockDevice: When partitioning, clear the rest of first erase unit #9384

merged 1 commit into from
Jan 16, 2019

Conversation

davidsaada
Copy link
Contributor

Description

In MBRBlockDevice, when performing the partition operation, make sure that the rest of the first erase unit (one that holds the partition table) is cleared. This serves for ensuring that no other data (like previously programmed file systems) is present in this erase unit. Trigger for this PR was a failures in CI, which alternately programmed LittleFS (without MBR) and FAT FS (with MBR), making LittleFS think that the FAT format was a valid one.

Pull request type

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

@davidsaada
Copy link
Contributor Author

@dannybenor @jenia81

@0xc0170 0xc0170 requested a review from a team January 15, 2019 14:39
NirSonnenschein
NirSonnenschein reviewed Jan 15, 2019
View reviewed changes
features/storage/blockdevice/MBRBlockDevice.cpp Outdated
}
if (table_start_offset + sizeof(struct mbr_table) < buffer_size) {
memset(buffer + table_start_offset + sizeof(struct mbr_table), 0xFF,
buffer_size - (table_start_offset + + sizeof(struct mbr_table)));
Copy link
Contributor

Choose a reason for hiding this comment

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

    • ?

Copy link
Contributor

Choose a reason for hiding this comment

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

* * ?

@NirSonnenschein ??

Copy link
Contributor

Choose a reason for hiding this comment

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

double plus ?

Copy link
Contributor

@NirSonnenschein NirSonnenschein Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

looks like a typo:
buffer_size - (table_start_offset + + sizeof(struct mbr_table)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Fixed. Not a bug though. :)

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2019

This targeting 5.11.2 ,correct?

@adbridge
Copy link
Contributor

@davidsaada Why aren't you using the Reviewers section in the template ? Designed specifically so that individuals can be tagged and have the script auto assign them as reviewers . Is your Master out of date ?

MBRBlockDevice: When partitioning, clear the rest of first erase unit
af23e3f 
Make sure all the parts of the first erase unit, that are not part of the
partition table are clear.
@davidsaada
Copy link
Contributor Author

This targeting 5.11.2 ,correct?

Indeed.

NirSonnenschein
NirSonnenschein approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@NirSonnenschein NirSonnenschein left a comment

Choose a reason for hiding this comment

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

looks ok to me

@davidsaada
Copy link
Contributor Author

@davidsaada Why aren't you using the Reviewers section in the template ? Designed specifically so that individuals can be tagged and have the script auto assign them as reviewers . Is your Master out of date ?

Thanks @adbridge I'm aware of it. Those folks were quoted there not for review sake, but in order for them to use this PR for the tests. Would like the "regular" folks to review this PR.

@NirSonnenschein NirSonnenschein removed the request for review from jenia81 January 15, 2019 14:49
@JanneKiiskila
Copy link
Contributor

Excellent, can we pull this to the 5.11.2 RC? @adbridge

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2019

Let me prepare the CI queue for this one to get in asap (while travis finishes)

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2019

CI started

@adbridge
Copy link
Contributor

Excellent, can we pull this to the 5.11.2 RC? @adbridge

@JanneKiiskila Once this passes CI we will cherry pick across to the RC for 5.11.2. Once that has been done we will need to re-run CI on the RC. Can we have someone also on standby to do the same for the client tests ?

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr merged commit 134905b into ARMmbed:master Jan 16, 2019
@cmonr cmonr removed the needs: CI label Jan 16, 2019
cmonr pushed a commit that referenced this pull request Jan 16, 2019
@cmonr
Merge pull request #9384 from davidsaada/david_mbr_erase_start
a2220f3 
MBRBlockDevice: When partitioning, clear the rest of first erase unit
@cmonr cmonr mentioned this pull request Jan 16, 2019
Merged
@davidsaada davidsaada deleted the david_mbr_erase_start branch February 22, 2019 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbridge adbridge adbridge left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@dannybenor dannybenor dannybenor approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@davidsaada @0xc0170 @adbridge @JanneKiiskila @mbed-ci @dannybenor @NirSonnenschein @cmonr