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

gh-114917: add support for AI_NUMERICSERV in getaddrinfo emulation #114918

Merged
merged 6 commits into from
Mar 18, 2025

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented Feb 2, 2024

Copy link
Contributor

@rruuaanng rruuaanng left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Can you write a test for this feature?

@maxbachmann
Copy link
Contributor Author

maxbachmann commented Mar 11, 2025

I am not sure which currently tested target even runs this code. This is only included conditionally via:

#if !defined(HAVE_GETNAMEINFO)
#define getnameinfo fake_getnameinfo
#include "getnameinfo.c"
#endif // HAVE_GETNAMEINFO

@maxbachmann
Copy link
Contributor Author

maxbachmann commented Mar 11, 2025

I added a test. However this only tests the fallback if we actually have a target using it that is under testing.
We use this fallback implementation on the playstation but we don't have unit tests running there.

@maxbachmann

This comment was marked as off-topic.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just have a minor coding style request.

@maxbachmann
Copy link
Contributor Author

@vstinner is there still something stopping this from getting merged?

@vstinner
Copy link
Member

@vstinner is there still something stopping this from getting merged?

I don't know Modules/getaddrinfo.c. Which platforms are using it? It looks like dead code to me. It starts with a big #if 0 and then most code is disabled unless the HAVE_NETDB_H macro is defined.

My pyconfig.h has:

/* Define to 1 if you have the <netdb.h> header file. */
#define HAVE_NETDB_H 1

but Modules/getaddrinfo.c doesn't include pyconfig.h nor Python.h.

@vstinner
Copy link
Member

I tried to using this Linux with the patch:

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
index 916ad35623e..c49d0d53042 100644
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -448,6 +448,8 @@ remove_unusable_flags(PyObject *m)
 
 #endif
 
+#undef HAVE_GETADDRINFO
+
 /* I know this is a bad practice, but it is the easiest... */
 #if !defined(HAVE_GETADDRINFO)
 /* avoid clashes with the C library definition of the symbol. */

On Linux, I get many build errors:

./Modules/getaddrinfo.c:206:30: error: 'EAI_MAX' undeclared (first use in this function); did you mean 'AF_MAX'?
./Modules/getaddrinfo.c:285:17: error: 'EAI_BADHINTS' undeclared (first use in this function); did you mean 'EAI_BADFLAGS'?
./Modules/getaddrinfo.c:286:32: error: 'AI_MASK' undeclared (first use in this function)
./Modules/getaddrinfo.c:354:19: error: invalid type argument of '->' (have 'struct addrinfo')
./Modules/getaddrinfo.c:384:25: error: 'EAI_PROTOCOL' undeclared (first use in this function); did you mean 'SO_PROTOCOL'?
./Modules/getaddrinfo.c:512:10: error: implicit declaration of function 'getipnodebyaddr'; did you mean 'getnetbyaddr'? [-Wimplicit-function-declaration]
./Modules/getaddrinfo.c:512:8: error: assignment to 'struct hostent *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
./Modules/getaddrinfo.c:561:14: error: implicit declaration of function 'getipnodebyname'; did you mean 'getprotobyname'? [-Wimplicit-function-declaration]
./Modules/getaddrinfo.c:561:12: error: assignment to 'struct hostent *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
./Modules/getaddrinfo.c:564:12: error: assignment to 'struct hostent *' from 'int' makes pointer from integer without a cast [-Wint-conversion]

How did you test your change? Are using using macOS?

@maxbachmann
Copy link
Contributor Author

I don't know Modules/getaddrinfo.c. Which platforms are using it? It looks like dead code to me. It starts with a big #if 0 and then most code is disabled unless the HAVE_NETDB_H macro is defined.

It's only used when HAVE_GETADDRINFO isn't defined. In this case it's directly including the c file and so doesn't need the imports.

On Linux, I get many build errors:

I assume you didn't undef this for enough code. The include of addrinfo.h in line 423 would need this define to already be removed.

How did you test your change? Are using using macOS?

We are using it in our playstation port of CPython

@vstinner vstinner merged commit 3453b5c into python:main Mar 18, 2025
42 checks passed
@vstinner
Copy link
Member

We are using it in our playstation port of CPython

Oh ok. A few lines to support PlayStation sounds reasonable, I merged your PR. Thanks.

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.

3 participants