-
Notifications
You must be signed in to change notification settings - Fork 214
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
Add an additional MB of space to the generated FAT partition #397
Conversation
src/fat.rs
Outdated
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB; | ||
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + MB) * MB; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that increasing the size by a terabyte? Note that the expression in the parentheses is already multiplied by MB
, so now it ends up being (... + MB) * MB
, which should be a TB if I'm not mistaken. Replacing the 1 by a 2 is probably a more storage efficient fix 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, doing something else then, hold on…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I decided to revert it back to what it was before and instead simply append + MB
to the end of that long calculation, since adding a megabyte is far more reasonable than multiplying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got this down to one commit but it took git reset --hard <the first of my previous commits>
and git pull https://github.com/rust-osdev/bootloader main --allow-unrelated-histories --rebase
to do it.
Add MB instead of multiplying
Could you remove the first commit? |
A revert commit is still a commit, ideally we'd only have a single commit for this pr. See https://stackoverflow.com/a/42522493 on how to remove commits without creating a revert commits. |
Add MB instead of multiplying
7297d1c
to
16624cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I updated the PR title to be more descriptive and added the line Fixes #396 to the PR description to automatically close the issue after merging.
One minor request: Right now the title of your commit is "Hard reset per @Freax13's request", which is not really useful. Could you change it to something more descriptive? An easy way to do this is to run the following command:
git commit --amend --message "Add an additional MB of space to the generated FAT partition"
src/fat.rs
Outdated
@@ -26,7 +26,7 @@ pub fn create_fat_filesystem( | |||
.truncate(true) | |||
.open(out_fat_path) | |||
.unwrap(); | |||
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB; | |||
let fat_size_padded_and_rounded = (((needed_size + 1024 * 64 - 1) / MB + 1) * MB) + MB; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it's more readable without the extra parentheses:
let fat_size_padded_and_rounded = (((needed_size + 1024 * 64 - 1) / MB + 1) * MB) + MB; | |
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB + MB; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to use a hard reset and force-push to incorporate both of these changes without also inadvertently turning 1 commit into 2, but done.
16624cd
to
244ec1a
Compare
244ec1a
to
52c8338
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
By the way, I recommend to use |
Increasing the image size by an additional megabyte makes the image compile again
Fixes #396