improve: prepare fetch_posix.c for platforms that miss getaddrinfo()#131
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
sergio-nsk
force-pushed
the
sergio-nsk/fetch/1
branch
from
294e38d to
d298a89
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #131 +/- ##
==========================================
+ Coverage 58.57% 63.84% +5.27%
==========================================
Files 32 34 +2
Lines 2607 2918 +311
Branches 526 546 +20
==========================================
+ Hits 1527 1863 +336
+ Misses 745 709 -36
- Partials 335 346 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sergio-nsk
changed the title
miss getaddrinfo()
There was a problem hiding this comment.
Pull request overview
This PR improves portability of the POSIX networking code by adding a CMake-detected HAVE_NETINET_IN_H config define and including <netinet/in.h> where needed, and it refactors fetch_get() to use a cached sockaddr/length when creating and connecting the socket.
Changes:
- Add
HAVE_NETINET_IN_Hdetection in CMake and expose it viaproxyres_config.h. - Include
<netinet/in.h>conditionally in POSIX networking sources that rely on networking constants/types. - Refactor
fetch_get()socket/connect argument handling.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wpad_dhcp_posix.c | Conditionally includes <netinet/in.h> based on new config define. |
| proxyres_config.h.in | Adds HAVE_NETINET_IN_H to the generated config header template. |
| net_util.c | Conditionally includes <netinet/in.h> for improved portability. |
| fetch_posix.c | Refactors address/length handling for socket()/connect(). |
| CMakeLists.txt | Adds check_include_file(netinet/in.h ...) for non-Windows builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Create communication socket | ||
| sfd = socket(address_info->ai_family, address_info->ai_socktype, address_info->ai_protocol); | ||
| sfd = socket(addr->sa_family, SOCK_STREAM, IPPROTO_TCP); |
There was a problem hiding this comment.
The socket is created with hard-coded SOCK_STREAM/IPPROTO_TCP instead of using address_info->ai_socktype / address_info->ai_protocol from getaddrinfo(). This can reduce portability and risks mismatches if the resolver returns an entry with different parameters; prefer using the values returned by getaddrinfo() (or hints) when calling socket().
| sfd = socket(addr->sa_family, SOCK_STREAM, IPPROTO_TCP); | |
| sfd = socket(address_info->ai_family, address_info->ai_socktype, address_info->ai_protocol); |
There was a problem hiding this comment.
hint is initialized with these constants, so it's ok.
hints.ai_socktype = SOCK_STREAM;
hints.ai_protocol = IPPROTO_TCP;
There was a problem hiding this comment.
hint is initialized with these constants, so it's ok.
True, but I think the existing code is more semantically correct. The new code duplicates assumptions already encoded in the getaddrinfo() hints, so it’s a bit less idiomatic and slightly more brittle if those hints ever change.
There was a problem hiding this comment.
The target platform SDK does not have getaddrinfo and struct addrinfo.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fetch_posix.c (1)
40-41: Use an API-matched type foraddrlen.Line 41 copies
ai_addrlenintouint32_t, but that value is defined by the socket API and then fed straight intoconnect(). Since this refactor is about portability, I’d keep it behind a small platform alias (socklen_ton POSIX,inton Winsock) instead of baking in a 32-bit type.♻️ Possible follow-up
Near the platform shims:
`#ifdef` _WIN32 # define socketerr WSAGetLastError() # define ssize_t int +typedef int socket_addrlen_t; `#else` # define socketerr errno # define SOCKET int # define closesocket close +typedef socklen_t socket_addrlen_t; `#endif`At the declaration site:
- uint32_t addrlen = 0; + socket_addrlen_t addrlen = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fetch_posix.c` around lines 40 - 41, The variable addrlen should use the socket-API-matching type rather than uint32_t: replace the declaration of addrlen (and any copies from ai_addrlen) with the platform alias (socklen_t on POSIX, int on Winsock) so the value passed to connect() matches the OS API; update the declaration near "struct sockaddr *addr" and any assignments from ai_addrlen to use that alias (or socklen_t) and adjust any casts or calls (e.g., connect()) accordingly to remove the incorrect uint32_t usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fetch_posix.c`:
- Around line 40-41: The variable addrlen should use the socket-API-matching
type rather than uint32_t: replace the declaration of addrlen (and any copies
from ai_addrlen) with the platform alias (socklen_t on POSIX, int on Winsock) so
the value passed to connect() matches the OS API; update the declaration near
"struct sockaddr *addr" and any assignments from ai_addrlen to use that alias
(or socklen_t) and adjust any casts or calls (e.g., connect()) accordingly to
remove the incorrect uint32_t usage.
d298a89 to
7cc601b
Compare
sergio-nsk
changed the title
miss getaddrinfo()getaddrinfo()
|
Maybe I should make constants. |
This reduces amount of #if blocks.
sergio-nsk
force-pushed
the
sergio-nsk/fetch/1
branch
from
7cc601b to
2940b23
Compare
This reduces amount of
#ifblocks.Summary by CodeRabbit