Skip to content

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.

colesbury pushed a commit to colesbury/cpython that referenced this pull request Mar 20, 2025
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
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