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

Various portability fixes #7861

Closed
wants to merge 3 commits into from
Closed

Conversation

fossdd
Copy link
Contributor

@fossdd fossdd commented Mar 3, 2025

See commit messages for further information

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Mar 3, 2025
@andreboscatto andreboscatto requested a review from sumit-bose March 4, 2025 13:35
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the fixes, I'm fine with them. Just a comment about the commit messages, afaik "quirk" is written with a "k".

bye,
Sumit

@fossdd fossdd force-pushed the portability-fixes branch from 9d16793 to be167ec Compare March 5, 2025 10:35
@fossdd
Copy link
Contributor Author

fossdd commented Mar 5, 2025

fixed, thanks!

@@ -45,6 +45,14 @@ typedef int errno_t;
#define EOK 0
#endif

#ifndef NETDB_INTERNAL
#define NETDB_INTERNAL (-1)
Copy link
Member

Choose a reason for hiding this comment

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

What does musl use instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

But how will musl work with 'libnss_sss' if it doesn't expect -1 to be returned?

Copy link
Contributor Author

@fossdd fossdd Mar 6, 2025

Choose a reason for hiding this comment

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

Hmm, do you know where libnss_sss returns NETDB_INTERNAL to musl?

Copy link
Member

Choose a reason for hiding this comment

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

$ grep -rn NETDB_INTERNAL *
nss_hosts.c:233:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:240:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:247:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:282:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:294:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:353:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:360:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:367:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:412:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:424:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:461:        *h_errnop = NETDB_INTERNAL;
nss_hosts.c:479:            *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:204:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:211:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:223:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:246:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:266:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:311:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:318:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:327:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:349:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:372:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:392:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:421:        *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:444:            *h_errnop = NETDB_INTERNAL;
nss_ipnetworks.c:467:        *h_errnop = NETDB_INTERNAL;

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why above patch was so contained.

Copy link
Member

Choose a reason for hiding this comment

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

@fossdd, what is your stance on this?
Do you want to dig deeper or are you fine to merely get code to the state "it compiled with musl"?

@sumit-bose, do you think we could extend 1a1e914 to replace all uses of NETDB_INTERNAL with NO_RECOVERY / HOST_NOT_FOUND (portable)?

@scabrero, do you remember if there was a reason to use NETDB_INTERNAL specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, it's fine. we already ship this code downstream, and afaik this snippet worked fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@scabrero, do you remember if there was a reason to use NETDB_INTERNAL specifically?

I don't remember if there was a specific reason by that time, but having a look it is probably because it is the way to say "check errno":
# define NETDB_INTERNAL -1 /* See errno. */

Copy link
Member

Choose a reason for hiding this comment

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

For me, it's fine. we already ship this code downstream, and afaik this snippet worked fine.

Ok, if nobody else chimes in in a next day or two, this will be merged "as is".

fossdd added 2 commits March 6, 2025 13:31
Remove usage of _PATH_HOSTS, which only exists in glibc
NETDB_INTERNAL is a glibc quirk and doesnt exist in POSIX
@fossdd fossdd force-pushed the portability-fixes branch from be167ec to 839847c Compare March 6, 2025 12:33
@alexey-tikhonov
Copy link
Member

Pushed PR: #7861

  • master
    • 459cc6b - CLIENT: Define NETDB_INTERNAL if not already
    • 8886a27 - failover: Clarify message for local hosts file resolution failure
    • 8edb14f - MC: Use useconds_t instead of their reserved type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants