os syscalls: add getifaddrs to singleton#18821
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
|
|
||
| // Small struct to avoid exposing ifaddrs -- which is not defined in all platforms -- to the | ||
| // codebase. | ||
| struct InterfaceAddress { |
There was a problem hiding this comment.
happy to move this to address.h if we think it makes more sense there. I couldn't decide.
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@mattklein123 still not done updating. I will ping you when this is ready for review. |
Signed-off-by: Jose Nino <jnino@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small comments, thanks.
/wait
envoy/api/os_sys_calls.h
Outdated
| // Small struct to avoid exposing ifaddrs -- which is not defined in all platforms -- to the | ||
| // codebase. | ||
| struct InterfaceAddress { | ||
| InterfaceAddress(absl::string_view ifa_name, unsigned int ifa_flags, |
There was a problem hiding this comment.
nit: can you just s/ifa/interface. It's easier to read.
| // TODO: eliminate this branching by upstreaming an alternative Android implementation | ||
| // e.g.: https://github.com/envoyproxy/envoy-mobile/blob/main/third_party/android/ifaddrs-android.h | ||
| #if defined(__ANDROID_API__) && __ANDROID_API__ < 24 | ||
| if (alternate_getifaddrs_.has_value()) { |
There was a problem hiding this comment.
Shouldn't you be doing this in all cases otherwise the interface docs are misleading?
There was a problem hiding this comment.
@mattklein123 I think I might be misunderstanding your point. Where else should I be checking this that is not checked?
There was a problem hiding this comment.
You should check this outside of the #ifdef ?
There was a problem hiding this comment.
I see what you mean now @mattklein123. My thought was that in posix the class would default to true for supporting getifaddrs regardless of if an alternate implementation is given. Only for Android<24 do we need to check for existence of an alternate implementation.
There was a problem hiding this comment.
OK I see. Makes sense. Can you just add a small comment?
/wait
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
3 participants
Commit Message: os syscalls - add getifaddrs to singleton
Additional Description: this PR reverts #18566 and adjusts code in order to be able to compile against android in Envoy Mobile
Risk Level: low, new API. Also tested against existing code.
Testing: existing UT.
Signed-off-by: Jose Nino jnino@lyft.com