Draft Android implementation of the new platform DNS resolver [chromium/src : main]
Vladyslav Kasprov (Gerrit)
Vladyslav Kasprov added 9 comments![]( "Open in Gerrit")
Draft Android implementation of the new platform DNS resolver with HTTPS DNS records support. Use android_res_nquery API to implement the resolution.Vladyslav KasprovFor Chromium's commit messages, usually first line is a brief summary of up to 50-80 chars, see [this](https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#uploading-a-change-for-review).
Then it's a blank line, followed by a more in-depth explanation.
In this case it would be useful in the in-depth explanation to call out that the parsing code for android_res_nresult is throwaway code that will later be substituted with the pre-existing async DNS resolver parsing logic.
Updated the commit message to have a brief summary followed by a blank line and in-depth explanation of in-scope/out of scope things for this CL.
Draft Android implementation of the new platform DNS resolver with HTTPS DNS records support. Use android_res_nquery API to implement the resolution.Vladyslav KasprovThis commit first line is too long. More generally, see https://cbea.ms/git-commit/
Updated the commit message to have a brief summary followed by a blank line and in-depth explanation.
class NET_EXPORT HostResolverPlatformSystemTaskVladyslav KasprovCan you add a brief comment above stating what this represents? I'm thinking something along the lines of:
```suggestion
// Implements `HostResolverSource::SYSTEM`, similarly to `HostResolverSystemTask`.
// Resolves hostnames querying the system/OS via platform specific APIs, instead of
// getaddrinfo.
class NET_EXPORT HostResolverPlatformSystemTask
```
Vladyslav KasprovUpdated the comment explaining the purpose of this class.
Does it look good now?
Updated the comment to:
```
Implements DNS resolution using platform specific APIs.
Implements `HostResolverSource::SYSTEM`, similarly to
`HostResolverSystemTask`. However, instead of getaddrinfo, resolves hostnames
using different platform-specific APIs that query the system/OS.
This class is designed to be used only for a single, one-shot hostname
resolution. This class is not thread-safe.
Simply call Start() to start the resolution. The callback will be called on
the thread that called Start() with the results of the resolution.
```
Resolving this thread for now. Let me know if there are more comments.
// synchronously and can own `this`.Vladyslav KasprovThis wording is a bit confusing. Did you mean "can destroy `this`"?
Updated the comment to:
```
Starts the resolution task. This can only be called once per
HostResolverPlatformSystemTask. `result_callback` will be invoked
asynchronously and can destroy `this`.
```
std::vector<std::string> extractIpAddressAnswers(uint8_t* buf,
size_t bufLen,Vladyslav KasprovEvery time you have something in the form of "pointer to an array and size", you'd be better off using `std::span` to represent it. https://abseil.io/tips/93
Both `std::span` and `absl::Span` are banned in Chromium, in favor of `base::span`. Using `base::span` changes the call site from:
```
ExtractIpAddressAnswers(answer_buf, answer_len, AF_INET);
```
to:
```
ExtractIpAddressAnswers(UNSAFE_BUFFERS(base::span<uint8_t>(answer_buf, static_cast<size_t>(answer_len))), AF_INET);
```
Do you still think we should make this change?
if (ns_initparse((const uint8_t*)buf, bufLen, &handle) < 0) {Vladyslav KasprovNit: it's a bit more robust to write `!= 0` - anything other than zero (even positive numbers) is unexpected, and should be treated as an error. According to that function code it should never return anything greater than zero.
Yes, `0` return value of that function stands for `success`, so anything else is a failure. Changed to `!= 0`
ns_rr rr;Vladyslav KasprovMove this declaration further down, closer to where it's used. https://google.github.io/styleguide/cppguide.html#Local_Variables
Moved inside the for-loop
for (int i = 0; i < ancount; i++) {Vladyslav Kasprov++i
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
Changed to `++i`
DCHECK(dns_names_util::IsValidDnsName(hostname_))Vladyslav KasprovUse `CHECK`, not `DCHECK`, unless you can show this is too expensive to run in production. https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#check_dcheck_and-notreached
Do not use existing code as precedent for this. Chromium has historically been infamously bad at using DCHECKs correctly.
Same below, many times.
Changed to `CHECK` and updated documentation. Now if the hostname is invalid, the constuctor will crash. Also, now if `Start` is called multiple times, it will crash on the second call. Left `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_)` as is, since I'm not sure if it should be changed or not.
Related details
- Etienne Dechamps
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladyslav Kasprov (Gerrit)
Vladyslav Kasprov added 5 comments![]( "Open in Gerrit")
std::string hostname,Vladyslav KasprovCan you document that this will crash if `hostname` is not valid (similarly to how you would document that a function throws IllegalArgumentException in Java for invalid arguments).
Added the following comment to the constructor:
```
`hostname` should be a valid domain name, and should be checked early
before calling this constructor. Will crash if `hostname` is invalid.
```
std::vector<std::string> extractIpAddressAnswers(uint8_t* buf,
size_t bufLen,Vladyslav KasprovEvery time you have something in the form of "pointer to an array and size", you'd be better off using `std::span` to represent it. https://abseil.io/tips/93
Both `std::span` and `absl::Span` are banned in Chromium, in favor of `base::span`. Using `base::span` changes the call site from:
```
ExtractIpAddressAnswers(answer_buf, answer_len, AF_INET);
```
to:
```
ExtractIpAddressAnswers(UNSAFE_BUFFERS(base::span<uint8_t>(answer_buf, static_cast<size_t>(answer_len))), AF_INET);
```Do you still think we should make this change?
Resolving for now. Let me know if we should still make this change.
DCHECK(dns_names_util::IsValidDnsName(hostname_))Vladyslav KasprovUse `CHECK`, not `DCHECK`, unless you can show this is too expensive to run in production. https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#check_dcheck_and-notreached
Do not use existing code as precedent for this. Chromium has historically been infamously bad at using DCHECKs correctly.
Same below, many times.
Changed to `CHECK` and updated documentation. Now if the hostname is invalid, the constuctor will crash. Also, now if `Start` is called multiple times, it will crash on the second call. Left `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_)` as is, since I'm not sure if it should be changed or not.
Resolving for now. Let me know if `DCHECK_CALLED_ON_VALID_SEQUENCE` should be addressed as well.
std::unique_ptr<HostResolverPlatformSystemTask> task_;Vladyslav Kasprov`unique_ptr` is overkill for this purpose. Use `std::optional`. https://abseil.io/tips/123 https://abseil.io/tips/187
After you move this to a local variable (see other comment), you can go even further and make it a bare object.
Moved `task` to a local variable and made it a bare object
std::unique_ptr<HostResolverPlatformSystemTask> task_;Vladyslav KasprovAvoid putting stuff in fixtures if you can. Here you could just make this a local variable inside the test. (Then you could just get rid of the fixture altogether, although I don't think you can as presumably you still need `TestWithTaskEnvironment`.)
More generally, don't use class member variables for things that could be local variables instead.
Moved `task_` into a local variable.
Replaced `TestWithTaskEnvironment` with a regular `::testing::Test`.
Related details
- Etienne Dechamps
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladyslav Kasprov (Gerrit)
Vladyslav Kasprov added 6 comments![]( "Open in Gerrit")
TRACE_EVENT0(NetTracingCategory(),
"HostResolverPlatformSystemTask::OnLookupComplete");Vladyslav KasprovIf you're going to add trace events (and you should), then you may as well sprinkle them everywhere, e.g. in Start().
Removed tracing events for now
if (*net_error != OK && NetworkChangeNotifier::IsOffline()) {
*net_error = ERR_INTERNET_DISCONNECTED;Vladyslav KasprovJust making sure: is it really this layer's responsibility to check for this? This seems like something that could/should be checked at a higher layer, with this layer merely returning the raw error from the OS.
We want to keep this, because [the existing system resolver does this too](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/net/dns/host_resolver_system_task.cc#449). We want to have the new resolver to be a drop-in replacement of the old one, and keep the same API. This `net_error` code is part of the API because it is passed to the caller in the result callback.
net_log_.EndEvent(NetLogEventType::HOST_RESOLVER_SYSTEM_TASK, [&] {Vladyslav KasprovThis should rely on a new NetLogEventType::HOST_RESOLVER_PLATFORM_SYSTEM_TASK.
Event types are defined [here](https://crsrc.org/c/net/log/net_log_event_type_list.h;l=195-211;drc=ae249e81ef263546d4b6d1cd5fbb5101e6f94374).
Removed net logs.
NetLogEventType::HOST_RESOLVER_MANAGER_ATTEMPT_FINISHED,Vladyslav Kasprov[HOST_RESOLVER_MANAGER_ATTEMPT_{STARTED, FINISHED}'s documentation](https://crsrc.org/c/net/log/net_log_event_type_list.h;l=138-161;drc=ae249e81ef263546d4b6d1cd5fbb5101e6f94374) seems out of date (HostResolverManager::ProcTask does not exist anymore?).
Having said that, it seems that it's used only by [HostResolverSystemTask::StartLookupAttempt](https://source.chromium.org/search?q=HOST_RESOLVER_MANAGER_ATTEMPT_STARTED&sq=&ss=chromium%2Fchromium%2Fsrc).
Removed net logs.
return NetLogFailedParams(0, *net_error, *os_error);
});
net_log_.AddEvent(
NetLogEventType::HOST_RESOLVER_MANAGER_ATTEMPT_FINISHED,
[&] { return NetLogFailedParams(1, *net_error, *os_error); });
} else {
net_log_.EndEvent(NetLogEventType::HOST_RESOLVER_SYSTEM_TASK,
[&] { return results->NetLogParams(); });
net_log_.AddEventWithIntParams(
NetLogEventType::HOST_RESOLVER_MANAGER_ATTEMPT_FINISHED,
"attempt_number", 1);Vladyslav KasprovI'm a bit confused as to how we are populating "attempt_number" here. It looks like we are only attempting once? Then why do we even populate this field in the netlog? Also, Why do we set it to 0 in `HOST_RESOLVER_SYSTEM_TASK` but 0 in `HOST_RESOLVER_MANAGER_ATTEMPT_FINISHED`?
Removed net logs altogether. We won't be adding them for now.
std::unique_ptr<HostResolverPlatformSystemTask> task_;Avoid putting stuff in fixtures if you can. Here you could just make this a local variable inside the test. (Then you could just get rid of the fixture altogether, although I don't think you can as presumably you still need `TestWithTaskEnvironment`.)
More generally, don't use class member variables for things that could be local variables instead.
Moved `task_` into a local variable.
Replaced `TestWithTaskEnvironment` with a regular `::testing::Test`.
Reverted `TestWithTaskEnvironment` change because `RunLoop` requires it.
`task_` is still a local variable.
Related details
- Etienne Dechamps
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladyslav Kasprov (Gerrit)
Vladyslav Kasprov added 10 comments![]( "Open in Gerrit")
constexpr int MAXPACKET = 8 * 1024;Vladyslav KasprovNit: somewhat of a (controversial) personal opinion, but I would not usually define constants that are only used once - all that does is inflict a back-and-forth across the file on the unfortunate reader for no real benefit.
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
constexpr int MAXPACKET = 8 * 1024;Vladyslav KasprovCan you document what this is, and how the value was chosen?
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
continue;Vladyslav KasprovMaybe `VLOG(1)` in this case, for ease of troubleshooting?
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
char buffer[INET6_ADDRSTRLEN];Vladyslav KasprovHow do you know the address is `AF_INET6`?
If the address is actually `AF_INET` then this is the wrong size, and will produce the wrong result (if it doesn't fail outright in `inet_ntop`).
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
if (inet_ntop(ipType, (const char*)rdata, buffer, sizeof(buffer))) {Vladyslav KasprovNit: it would be more robust to write `== buffer`, which is what the function is expected to return.
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
answers.push_back(buffer);Vladyslav KasprovThis will incur an unnecessary copy into an `std::string`.
A more efficient way would be:
```
std::string buffer(INET6_ADDRSTRLEN);
inet_ntop(..., buffer.data(), buffer.size());
answers.push_back(std::move(buffer));
```
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
// This line is important to keep to avoid internal MessagePumpEpoll crash.Vladyslav KasprovCan you add more detail here? Why does it crash otherwise? What is the API contract here?
This will be investigated as part of a separate bug (https://crbug.com/450545129)
const auto ip_address = IPAddress::FromIPLiteral(answer);Vladyslav KasprovThis is round-tripping the IP address from binary (bytes) to string and then back to bytes? Wouldn't it be more straightforward and efficient to just create the IPAddress directly from the raw bytes? It would also eliminate the possibility that this could fail (i.e. your if below)
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
if (ip_address) {Vladyslav KasprovMaybe `VLOG(1)` in this is false, for ease of troubleshooting?
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
addr_list->push_back(IPEndPoint(*ip_address, 0));Vladyslav KasprovReturning a zero port seems... odd? Can you add a comment to explain why this is fine?
This code is part of the throwaway parsing logic, which is going to be replaced with the proper parsing logic in the future CLs. See more details in [crbug.com/449966580](https://crbug.com/449966580) and [go/https-rr-android-design-doc](go/https-rr-android-design-doc#heading=h.at7a3712jww9).
Related details
- Stefano Duo
- Vladyslav Kasprov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Dechamps (Gerrit)
Etienne Dechamps added 6 comments![]( "Open in Gerrit")
// Implements DNS resolution using platform specific APIs.
//
// Implements `HostResolverSource::SYSTEM`, similarly to
// `HostResolverSystemTask`. However, instead of getaddrinfo, resolves hostnames
// using different platform-specific APIs that query the system/OS.
//
// This class is designed to be used only for a single, one-shot hostname
// resolution. This class is not thread-safe.
//
// Simply call Start() to start the resolution. The callback will be called on
// the thread that called Start() with the results of the resolution.
class NET_EXPORT HostResolverPlatformSystemTask finalI think this is too generic. It should have "Android" in the file and class name (I would even go as far as to call it `AndroidResNquerySystemTask`). There is no way this class can be reused across platforms - starting with the Android-specific `__INTRODUCED_IN` attributes, and the whole fd watching stuff.
That is not to say we shouldn't have a platform-agnostic abstraction, but this is clearly not that, and can be introduced later (e.g. as an abstract class).
~HostResolverPlatformSystemTask() override;Etienne DechampsYou can drop the override here since this class is not being extended (I also don't think we plan to).
Vladyslav KasprovThe override is because the class is implementing FdWatcher. But even then, it is true that override is typically not useful on destructors anyway.
Removing the `override` results in a [modernize-use-override](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html) warning from `clang-tidy`:
```
Annotate this function with 'override' or (rarely) 'final'
```I marked it as `final` instead.
Does it look good now?
Vladyslav Kasprov
Etienne DechampsResolving this comment for now.
Let me know if this issue wasn't fully addressed, and more changes are needed.
If clang-tidy complains, just leave it as `override`. `final` is technically fine here, but is weird and unusual.
int errno_copy = errno;Can you point me to the doc on this `WatchFileDescriptor` that states that errors are surfaced through errno?
If there is no such doc, I wouldn't assume they are, and instead treat it as an unknown error.
// TODO: This should never happen. Add a trace event to detect if it does.I think you should downright CHECK-fail on this. Given you annotated your public API with `__INTRODUCED_IN(29)`, it is the responsibility of the user of the class to ensure they do not use this class on API < 29. Besides, we shouldn't even be able to get here if API < 29.
if (*net_error != OK && NetworkChangeNotifier::IsOffline()) {
*net_error = ERR_INTERNET_DISCONNECTED;Vladyslav KasprovJust making sure: is it really this layer's responsibility to check for this? This seems like something that could/should be checked at a higher layer, with this layer merely returning the raw error from the OS.
We want to keep this, because [the existing system resolver does this too](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/net/dns/host_resolver_system_task.cc#449). We want to have the new resolver to be a drop-in replacement of the old one, and keep the same API. This `net_error` code is part of the API because it is passed to the caller in the result callback.
Can you explain this in a code comment?
FAIL() << "This test is failing because it has to be run on Android 29+.";I don't think this will pass CQ, as it will try your test on older versions of Android (in addition to recent ones).
What you want is GTEST_SKIP() instead. https://google.github.io/googletest/reference/testing.html#GTEST_SKIP unless there is a better way to restrict a gUnit test to specific Android API versions that I am not aware of.
Related details
- Stefano Duo
- Vladyslav Kasprov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladyslav Kasprov (Gerrit)
Vladyslav Kasprov added 6 comments![]( "Open in Gerrit")
// Implements DNS resolution using platform specific APIs.
//
// Implements `HostResolverSource::SYSTEM`, similarly to
// `HostResolverSystemTask`. However, instead of getaddrinfo, resolves hostnames
// using different platform-specific APIs that query the system/OS.
//
// This class is designed to be used only for a single, one-shot hostname
// resolution. This class is not thread-safe.
//
// Simply call Start() to start the resolution. The callback will be called on
// the thread that called Start() with the results of the resolution.
class NET_EXPORT HostResolverPlatformSystemTask finalI think this is too generic. It should have "Android" in the file and class name (I would even go as far as to call it `AndroidResNquerySystemTask`). There is no way this class can be reused across platforms - starting with the Android-specific `__INTRODUCED_IN` attributes, and the whole fd watching stuff.
That is not to say we shouldn't have a platform-agnostic abstraction, but this is clearly not that, and can be introduced later (e.g. as an abstract class).
Renamed to `PlatformDnsQueryExecutorAndroid`.
[go/https-rr-android-design-doc](go/https-rr-android-design-doc) proposes introducing `PlatformDnsQueryExecutor` base class responsible for resolving hostnames. It's implementations would be platform-specific.
The class introducted in this CL is basically an Android implementation of the proposed `PlatformDnsQueryExecutor`. Hence, I named it `PlatformDnsQueryExecutorAndroid`. Adding a platform suffix seems to be standard practice in Chromium. I took inspiration from an existing `DnsConfigService` class and its implementations.
~HostResolverPlatformSystemTask() override;Etienne DechampsYou can drop the override here since this class is not being extended (I also don't think we plan to).
Vladyslav KasprovThe override is because the class is implementing FdWatcher. But even then, it is true that override is typically not useful on destructors anyway.
Vladyslav KasprovRemoving the `override` results in a [modernize-use-override](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html) warning from `clang-tidy`:
```
Annotate this function with 'override' or (rarely) 'final'
```I marked it as `final` instead.
Does it look good now?
Etienne DechampsResolving this comment for now.
Let me know if this issue wasn't fully addressed, and more changes are needed.
If clang-tidy complains, just leave it as `override`. `final` is technically fine here, but is weird and unusual.
Reverted to `override`.
Can you point me to the doc on this `WatchFileDescriptor` that states that errors are surfaced through errno?
If there is no such doc, I wouldn't assume they are, and instead treat it as an unknown error.
I couldn't find any docs for `WatchFileDescriptor` itself. However.
Depending on the platform, `WatchFileDescriptor` has multiple implementations. On Android, it [resolves](https://chromium.googlesource.com/chromium/src/+/main/base/message_loop/message_pump_for_io.h#23) to [MessagePumpEpoll::WatchFileDescriptor](https://chromium.googlesource.com/chromium/src/+/main/base/message_loop/message_pump_epoll.h#128).
Internally, `MessagePumpEpoll::WatchFileDescriptor ` uses [epoll_ctl](https://litux.nl/man/htmlman2/epoll_ctl.2.html), which **is** documented to set `errno` appropriately on error.
// TODO: This should never happen. Add a trace event to detect if it does.I think you should downright CHECK-fail on this. Given you annotated your public API with `__INTRODUCED_IN(29)`, it is the responsibility of the user of the class to ensure they do not use this class on API < 29. Besides, we shouldn't even be able to get here if API < 29.
Changed this `if-else` to:
```
if (__builtin_available(android 29, *)) {
ReadPlatformResponseAndroid29(fd);
} else {
// The only time this class is expected to receive this callback is when
// reading a file descriptor returned by android_res_nquery() API, which is
// available only on Android 29+. Change this code if this class starts to
// read file descriptors for other purposes. Until then, this is a safety
// check to crash early if this code path is reached.
CHECK(false);
}
```
if (*net_error != OK && NetworkChangeNotifier::IsOffline()) {
*net_error = ERR_INTERNET_DISCONNECTED;Vladyslav KasprovJust making sure: is it really this layer's responsibility to check for this? This seems like something that could/should be checked at a higher layer, with this layer merely returning the raw error from the OS.
Etienne DechampsWe want to keep this, because [the existing system resolver does this too](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/net/dns/host_resolver_system_task.cc#449). We want to have the new resolver to be a drop-in replacement of the old one, and keep the same API. This `net_error` code is part of the API because it is passed to the caller in the result callback.
Can you explain this in a code comment?
Added the following comment:
```
// This logic is copied from `HostResolverSystemTask`. This class is designed
// as drop-in replacement for `HostResolverSystemTask`, and hence had to mimic
// its API. `net_error` is part of the API because it's returned to the user
// in the `result_callback_`.
```
FAIL() << "This test is failing because it has to be run on Android 29+.";I don't think this will pass CQ, as it will try your test on older versions of Android (in addition to recent ones).
What you want is GTEST_SKIP() instead. https://google.github.io/googletest/reference/testing.html#GTEST_SKIP unless there is a better way to restrict a gUnit test to specific Android API versions that I am not aware of.
Changed this `FAIL()` to:
```
GTEST_SKIP_("The class that's being tested is available only on Android 29+");
```
Related details
- Etienne Dechamps
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Dechamps (Gerrit)
Etienne Dechamps voted and added 2 comments![]( "Open in Gerrit")
Votes added by Etienne Dechamps
| Code-Review | +1 |
2 comments
int errno_copy = errno;Vladyslav KasprovCan you point me to the doc on this `WatchFileDescriptor` that states that errors are surfaced through errno?
If there is no such doc, I wouldn't assume they are, and instead treat it as an unknown error.
I couldn't find any docs for `WatchFileDescriptor` itself. However.
Depending on the platform, `WatchFileDescriptor` has multiple implementations. On Android, it [resolves](https://chromium.googlesource.com/chromium/src/+/main/base/message_loop/message_pump_for_io.h#23) to [MessagePumpEpoll::WatchFileDescriptor](https://chromium.googlesource.com/chromium/src/+/main/base/message_loop/message_pump_epoll.h#128).
Internally, `MessagePumpEpoll::WatchFileDescriptor ` uses [epoll_ctl](https://litux.nl/man/htmlman2/epoll_ctl.2.html), which **is** documented to set `errno` appropriately on error.
The problem with this is:
1. There is no guarantee the implementation will be careful to preserve errno for callers. It is very easy to trample errno - for example, just calling `LOG(...) << ...` will trample errno (because it triggers I/O). This makes this mechanism very brittle and the resulting error code may be subtly wrong in ways that may be hard to notice.
2. Even if the implementation preserves errno now, there is no guarantee that it will continue to do so, especially since that's not documented. We should not rely on undocumented behavior.
std::vector<std::string> answers =This variable is unnecessary, and could be inlined. https://abseil.io/tips/161
Related details
- Stefano Duo
- Vladyslav Kasprov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |