Fix getprotobyname missing on Android preventing use#470
Conversation
getprotobyname is stubbed out on Android causing crash_and_burn to be called. This commit removes the icmp support check when compiling for Android and adds a configuration option to compile without the check. Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option to disable the use of getprotobyname and switches to using hardcoded protocol constants (IPPROTO_ICMP and IPPROTO_ICMPV6) for socket initialization. This change improves compatibility with environments like Android where these lookups may fail. A review comment identifies formatting issues in src/socket6.c, specifically the use of tabs instead of spaces and a minor syntax typo in an if condition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
…k/fping into fix-getprotobyname-android
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces conditional compilation to bypass getprotobyname on platforms like Android, falling back to default protocol constants (IPPROTO_ICMP and IPPROTO_ICMPV6). The reviewer suggests a more robust and idiomatic approach using Autoconf's standard feature detection (AC_CHECK_FUNCS([getprotobyname])) to define HAVE_GETPROTOBYNAME automatically, rather than hardcoding platform-specific checks and using a custom USE_GETPROTOBYNAME variable.
|
coverage: 88.08% (+0.08%) from 88.0% — Paliak:fix-getprotobyname-android into schweikert:develop |
…sabling with android override Change as per schweikert#470 (comment)
| struct protoent* proto; | ||
| int s; | ||
| int s = -1; | ||
| int p_proto = IPPROTO_ICMP; |
There was a problem hiding this comment.
Can't we just use this on all platforms? It seems to me that maybe we don't need getprotobyname at all.
There was a problem hiding this comment.
That was my thinking too. @gsnw-sebast suggested there may be situations where IPPROTO_ICMP differs from the return value of getprotobyname, so I added it back per that suggestion.
If we really want to check whether ICMP is supported on a given platform maybe we could just check for the define?
There was a problem hiding this comment.
I looked into it, and you're actually right. The value could be set statically without the check via getprotobyname(). The numbers are assigned by the IANA [1], and even if fping were to support TCP or UDP in the future, these numbers would remain unchanged with TCP = 6 and UDP = 17.
It would even keep fping running if an incorrect protocol file were stored on the system
[1] https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
…k/fping into fix-getprotobyname-android
gsnw-sebast
left a comment
gsnw-sebast
left a comment
There was a problem hiding this comment.
As far as I'm concerned, we can go ahead with that. We just need to update the changelog and prepare for a rebase merge.
4 participants
getprotobynameis stubbed out on Android causing crash_and_burn to be called. This commit removes the ICMP support check when compiling for Android and adds a configuration option to compile withoutgetprotobynamecheck.