Skip to content

Pad boot image to block size #39

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

Merged
merged 5 commits into from
May 24, 2019
Merged

Pad boot image to block size #39

merged 5 commits into from
May 24, 2019

Conversation

toothbrush7777777
Copy link
Contributor

Closes #35.

@phil-opp
Copy link
Member

Thanks a lot! Looks good to me overall.

One small nit: Could you rename the first padding binding to something else, e.g. remainder? Then we wouldn't have two bindings with the same name but different semantics.

@toothbrush7777777
Copy link
Contributor Author

toothbrush7777777 commented May 24, 2019

@phil-opp Done. I also formatted the changes as suggested by cargo fmt -- --check.

@phil-opp
Copy link
Member

Thanks!

bors r+

bors bot added a commit that referenced this pull request May 24, 2019
39: Pad boot image to block size r=phil-opp a=toothbrush7777777

Closes #35.

Co-authored-by: Toothbrush <toothbrush7777777@gmail.com>
@bors
Copy link
Contributor

bors bot commented May 24, 2019

Canceled

@toothbrush7777777
Copy link
Contributor Author

toothbrush7777777 commented May 24, 2019

@phil-opp Whoops, looks like I shouldn't have formatted the code further.

@phil-opp
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request May 24, 2019
39: Pad boot image to block size r=phil-opp a=toothbrush7777777

Closes #35.

Co-authored-by: Toothbrush <toothbrush7777777@gmail.com>
@phil-opp
Copy link
Member

phil-opp commented May 24, 2019

bors r-

I think we can merge this manually (pending the CI checks).

@bors
Copy link
Contributor

bors bot commented May 24, 2019

Canceled

@phil-opp
Copy link
Member

Seems like the test failed on Windows:

Error: I/O error: failed to pad boot image to a multiple of the block size: Access is denied. (os error 5)

@toothbrush7777777
Copy link
Contributor Author

@phil-opp Any idea what the problem could be?

@toothbrush7777777
Copy link
Contributor Author

toothbrush7777777 commented May 24, 2019

Finally passes all checks!

I found a comment which seems related:

.append(true) doesn't set all the bits of GENERIC_WRITE, it misses the FILE_WRITE_DATA bit as @retep998 mentioned. […]

rust-lang/rust#54118 (comment)

I guess that whatever API is used internally by Rust for File::set_len on Windows requires that flag. I've changed the code to use write(true) instead of append(true) and all the checks have now successfully passed.

@phil-opp
Copy link
Member

Thanks for investigating! Let's get this merged!

@phil-opp phil-opp merged commit 7cc1c07 into rust-osdev:master May 24, 2019
@phil-opp
Copy link
Member

Thanks a lot for tackling this! I will publish a new version tomorrow when I'm at my computer again.

@toothbrush7777777 toothbrush7777777 deleted the patch-1 branch May 24, 2019 18:53
@toothbrush7777777
Copy link
Contributor Author

@phil-opp Cool, that will be great.

@phil-opp
Copy link
Member

Released as version 0.7.4.

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

Successfully merging this pull request may close these issues.

Support VirtualBox: Ensure that binary size is a multiple of 512 bytes
2 participants