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.

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

bootloader: clean up endianess conversion #5457

Merged
merged 2 commits into from Feb 13, 2021

Conversation

rokm
Copy link
Member

@rokm rokm commented Jan 8, 2021

This PR places the endianess conversion for multi-byte integer fields in COOKIE and TOC structures in a single place, i.e., after the data is read from the file (as opposed to performing it on every field access). The main motivation is to simplify the eventual transition from 32-bit integers to 64-bit ones to increase maximum archive file size.

On Windows, replace the use of ntohl() from ws2_32 library with compiler-provided byte-swap functions. This allows us to drop otherwise unneccessary dependency on ws2_32 in bootloader executables.

@rokm rokm marked this pull request as ready for review January 11, 2021 13:03
@htgoebel htgoebel added the area:bootloader Caused be or effecting the bootloader label Jan 11, 2021
@htgoebel htgoebel added this to the PyInstaller 5.0 milestone Jan 11, 2021
The conversion of multi-byte integer fields in COOKIE and TOC
structures is a data (de)serialization detail, and as such should
be performed only once, after the data is read; as opposed to
on-the-fly endianess conversions on every field access, which are
scattered throughout the codebase.
On Windows, use built-in byte-swap macros instead of ntohl(), which
is available from winsock2 library. Using the latter requires the
bootloader to be linked against the winsock2 library, which may have
other unintended consequences.

On all other platforms, continue using ntohl() for now, but hide it
behind the pyi_be32toh() macro.
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

I still can't believe the C world has gone this long without a universal endian library.

Copy link
Contributor

@BoboTiG BoboTiG left a comment

Choose a reason for hiding this comment

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

And nice refactoring 👍

@Legorooj Legorooj merged commit c24117a into pyinstaller:develop Feb 13, 2021
@rokm rokm deleted the bootloader-endian branch May 5, 2021 09:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:bootloader Caused be or effecting the bootloader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants