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

Fix PG protocol handling on big-endian #379

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix PG protocol handling on big-endian #379

wants to merge 1 commit into from

Conversation

df7cb
Copy link
Contributor

@df7cb df7cb commented Sep 6, 2023

Replace the homegrown swap-bytes machinery by system functions that work correctly independently from the host byte order. pgagroal_read/write_* were wrong on big-endian machines before.

Problem noticed on Debian s390x where connects to pgagroal were just hanging indefinitely.

@df7cb
Copy link
Contributor Author

df7cb commented Sep 7, 2023

Added missing #include <endian.h> in security.c now.

@jesperpedersen jesperpedersen added the bug Something isn't working label Sep 7, 2023
@jesperpedersen
Copy link
Collaborator

MacOS is failing with this pull request

@jesperpedersen
Copy link
Collaborator

Maybe #if HAVE_OSX and #include <machine/endian.h> ?

https://stackoverflow.com/questions/20813028/endian-h-not-found-on-mac-osx

@df7cb
Copy link
Contributor Author

df7cb commented Sep 7, 2023

I was googling around and found like half a dozen different answers. Will give this one a shot now.

@jesperpedersen
Copy link
Collaborator

Keep an eye on the CI environment

@df7cb df7cb force-pushed the bigendian branch 2 times, most recently from d21824a to b01631a Compare September 7, 2023 13:40
@jesperpedersen
Copy link
Collaborator

Its HAVE_OSX

@df7cb
Fix PG protocol handling on big-endian
157a000 
Replace the homegrown swap-bytes machinery by system functions that work
correctly independently from the host byte order. pgagroal_read/write_*
were wrong on big-endian machines before.
@jesperpedersen
Copy link
Collaborator

Should it be a "wrapper" include, or an #if for this ?

@df7cb
Copy link
Contributor Author

df7cb commented Sep 7, 2023

Sorry the commit where I had tried HAVE_OSX had a silly typo elsewhere. But it looks like __APPLE__ worked, and it found the header, but htobe32 is still not available.

@jesperpedersen
Copy link
Collaborator

@df7cb Any updates ?

@df7cb
Copy link
Contributor Author

df7cb commented Feb 23, 2024

I believe my patch is correct, but MacOS lacks the modern htobe* functions.

@jesperpedersen
Copy link
Collaborator

@df7cb Maybe we should merge this on master and see if somebody with a Mac steps up and fixes it such that it can be backported ?

Likely the issue is the same with pgmoneta and pgexporter...

@jesperpedersen
Copy link
Collaborator

Thinking about it some more - we should leave the existing functionality for #ifdef HAVE_OSX in, and use your changes for all other platforms.

Could you rework your patch for that ?

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

@jesperpedersen jesperpedersen Awaiting requested review from jesperpedersen

Assignees

@df7cb df7cb

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@df7cb @jesperpedersen