Fixup inputs and outputs in DnsConfigService

Updating everything to match the latest style guide preferences, mostly
using references much more liberally and converting output params to
returns wherever reasonable.

Also switching from copies to moves in a couple places that I happened
to notice, and a couple more random cleanups to make Tricium happy.

Change-Id: I999f511835e70e61ee8c0e7c5dd7e6b7f2396aea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626786Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844119}

net/dns/dns_config_service.cc

@@ -70,8 +70,8 @@ void DnsConfigService::RefreshConfig() {
   NOTREACHED();
 }
 
-DnsConfigService::Watcher::Watcher(DnsConfigService* service)
-    : service_(service) {}
+DnsConfigService::Watcher::Watcher(DnsConfigService& service)
+    : service_(&service) {}
 
 DnsConfigService::Watcher::~Watcher() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -92,31 +92,41 @@ void DnsConfigService::Watcher::CheckOnCorrectSequence() {
 }
 
 DnsConfigService::HostsReader::HostsReader(
-    DnsConfigService* service,
-    base::FilePath::StringPieceType hosts_file_path)
-    : service_(service), hosts_file_path_(hosts_file_path) {}
+    base::FilePath::StringPieceType hosts_file_path,
+    DnsConfigService& service)
+    : service_(&service), hosts_file_path_(hosts_file_path) {}
 
 DnsConfigService::HostsReader::~HostsReader() = default;
 
-bool DnsConfigService::HostsReader::ReadHosts(DnsHosts* out_dns_hosts) {
+base::Optional<DnsHosts> DnsConfigService::HostsReader::ReadHosts() {
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
   DCHECK(!hosts_file_path_.empty());
-  return ParseHostsFile(hosts_file_path_, out_dns_hosts);
+  DnsHosts dns_hosts;
+  if (!ParseHostsFile(hosts_file_path_, &dns_hosts))
+    return base::nullopt;
+
+  return dns_hosts;
 }
 
-bool DnsConfigService::HostsReader::AddAdditionalHostsTo(DnsHosts& dns_hosts) {
+bool DnsConfigService::HostsReader::AddAdditionalHostsTo(
+    DnsHosts& in_out_dns_hosts) {
   // Nothing to add in base implementation.
   return true;
 }
 
 void DnsConfigService::HostsReader::DoWork() {
-  success_ = ReadHosts(&hosts_) && AddAdditionalHostsTo(hosts_);
+  hosts_ = ReadHosts();
+  if (!hosts_.has_value())
+    return;
+
+  if (!AddAdditionalHostsTo(hosts_.value()))
+    hosts_.reset();
 }
 
 void DnsConfigService::HostsReader::OnWorkFinished() {
-  if (success_) {
-    service_->OnHostsRead(hosts_);
+  if (hosts_.has_value()) {
+    service_->OnHostsRead(std::move(hosts_).value());
   } else {
     LOG(WARNING) << "Failed to read DnsHosts.";
   }
@@ -128,7 +138,7 @@ void DnsConfigService::ReadHostsNow() {
   if (!hosts_reader_) {
     DCHECK(!hosts_file_path_.empty());
     hosts_reader_ =
-        base::MakeRefCounted<HostsReader>(this, hosts_file_path_.value());
+        base::MakeRefCounted<HostsReader>(hosts_file_path_.value(), *this);
   }
   hosts_reader_->WorkNow();
 }
@@ -149,7 +159,7 @@ void DnsConfigService::InvalidateHosts() {
   StartTimer();
 }
 
-void DnsConfigService::OnConfigRead(const DnsConfig& config) {
+void DnsConfigService::OnConfigRead(DnsConfig config) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(config.IsValid());
 
@@ -163,11 +173,11 @@ void DnsConfigService::OnConfigRead(const DnsConfig& config) {
     OnCompleteConfig();
 }
 
-void DnsConfigService::OnHostsRead(const DnsHosts& hosts) {
+void DnsConfigService::OnHostsRead(DnsHosts hosts) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (hosts != dns_config_.hosts) {
-    dns_config_.hosts = hosts;
+    dns_config_.hosts = std::move(hosts);
     need_update_ = true;
   }
 

net/dns/dns_config_service.h

@@ -80,7 +80,7 @@ class NET_EXPORT_PRIVATE DnsConfigService {
    public:
     // `service` is expected to own the created Watcher and thus stay valid for
     // the lifetime of the created Watcher.
-    explicit Watcher(DnsConfigService* service);
+    explicit Watcher(DnsConfigService& service);
     virtual ~Watcher();
 
     Watcher(const Watcher&) = delete;
@@ -108,11 +108,13 @@ class NET_EXPORT_PRIVATE DnsConfigService {
 
   // Reader of HOSTS files. In this base implementation, uses standard logic
   // appropriate to most platforms to read the HOSTS file located at
-  // `service->GetHostsFilePath()`.
+  // `hosts_file_path`.
   class HostsReader : public SerialWorker {
    public:
-    HostsReader(DnsConfigService* service,
-                base::FilePath::StringPieceType hosts_file_path);
+    // `service` is expected to own the created reader and thus stay valid for
+    // the lifetime of the created reader.
+    HostsReader(base::FilePath::StringPieceType hosts_file_path,
+                DnsConfigService& service);
 
     HostsReader(const HostsReader&) = delete;
     HostsReader& operator=(const HostsReader&) = delete;
@@ -120,19 +122,19 @@ class NET_EXPORT_PRIVATE DnsConfigService {
    protected:
     ~HostsReader() override;
 
-    // Reads the HOSTS file and parses to a `DnsHosts`. Returns false on
+    // Reads the HOSTS file and parses to a `DnsHosts`. Returns nullopt on
     // failure. Will be called on a separate blockable ThreadPool thread.
     //
     // Override if needed to implement platform-specific behavior, e.g. for a
     // platform-specific HOSTS format.
-    virtual bool ReadHosts(DnsHosts* out_dns_hosts);
+    virtual base::Optional<DnsHosts> ReadHosts();
 
     // Adds any necessary additional entries to the given `DnsHosts`. Returns
     // false on failure. Will be called on a separate blockable ThreadPool
     // thread.
     //
     // Override if needed to implement platform-specific behavior.
-    virtual bool AddAdditionalHostsTo(DnsHosts& dns_hosts);
+    virtual bool AddAdditionalHostsTo(DnsHosts& in_out_dns_hosts);
 
     // SerialWorker:
     void DoWork() final;
@@ -144,8 +146,7 @@ class NET_EXPORT_PRIVATE DnsConfigService {
     // running on worker thread.
     DnsConfigService* const service_;
     // Written in DoWork, read in OnWorkFinished, no locking necessary.
-    DnsHosts hosts_;
-    bool success_ = false;
+    base::Optional<DnsHosts> hosts_;
 
     const base::FilePath hosts_file_path_;
   };
@@ -162,9 +163,9 @@ class NET_EXPORT_PRIVATE DnsConfigService {
   void InvalidateHosts();
 
   // Called with new config. |config|.hosts is ignored.
-  void OnConfigRead(const DnsConfig& config);
+  void OnConfigRead(DnsConfig config);
   // Called with new hosts. Rest of the config is assumed unchanged.
-  void OnHostsRead(const DnsHosts& hosts);
+  void OnHostsRead(DnsHosts hosts);
 
   SEQUENCE_CHECKER(sequence_checker_);
 

net/dns/dns_config_service_posix.h

@@ -59,11 +59,9 @@ class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService {
 };
 
 #if !defined(OS_ANDROID)
-// Fills in |dns_config| from |res|. Returns false iff a valid config could not
-// be determined.
-bool NET_EXPORT_PRIVATE
-ConvertResStateToDnsConfig(const struct __res_state& res,
-                           DnsConfig* dns_config);
+// Returns nullopt iff a valid config could not be determined.
+base::Optional<DnsConfig> NET_EXPORT_PRIVATE
+ConvertResStateToDnsConfig(const struct __res_state& res);
 #endif
 
 }  // namespace internal

net/dns/dns_config_service_posix_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/cancelable_callback.h"
 #include "base/files/file_util.h"
+#include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
@@ -146,17 +147,16 @@ void InitializeExpectedConfig(DnsConfig* config) {
 
 TEST(DnsConfigServicePosixTest, ConvertResStateToDnsConfig) {
   struct __res_state res;
-  DnsConfig config;
-  EXPECT_FALSE(config.IsValid());
   InitializeResState(&res);
-  ASSERT_TRUE(internal::ConvertResStateToDnsConfig(res, &config));
+  base::Optional<DnsConfig> config = internal::ConvertResStateToDnsConfig(res);
   CloseResState(&res);
-  EXPECT_TRUE(config.IsValid());
+  ASSERT_TRUE(config.has_value());
+  EXPECT_TRUE(config->IsValid());
 
   DnsConfig expected_config;
-  EXPECT_FALSE(expected_config.EqualsIgnoreHosts(config));
+  EXPECT_FALSE(expected_config.EqualsIgnoreHosts(config.value()));
   InitializeExpectedConfig(&expected_config);
-  EXPECT_TRUE(expected_config.EqualsIgnoreHosts(config));
+  EXPECT_TRUE(expected_config.EqualsIgnoreHosts(config.value()));
 }
 
 TEST(DnsConfigServicePosixTest, RejectEmptyNameserver) {
@@ -175,12 +175,11 @@ TEST(DnsConfigServicePosixTest, RejectEmptyNameserver) {
   res.nsaddr_list[1] = sa;
   res.nscount = 2;
 
-  DnsConfig config;
-  EXPECT_FALSE(internal::ConvertResStateToDnsConfig(res, &config));
+  EXPECT_FALSE(internal::ConvertResStateToDnsConfig(res));
 
   sa.sin_addr.s_addr = 0xDEADBEEF;
   res.nsaddr_list[0] = sa;
-  EXPECT_TRUE(internal::ConvertResStateToDnsConfig(res, &config));
+  EXPECT_TRUE(internal::ConvertResStateToDnsConfig(res));
 }
 
 TEST(DnsConfigServicePosixTest, DestroyWhileJobsWorking) {

net/dns/dns_config_service_win.h

@@ -39,17 +39,15 @@ namespace net {
 namespace internal {
 
 // Converts a UTF-16 domain name to ASCII, possibly using punycode.
-// Returns true if the conversion succeeds and output is not empty. In case of
-// failure, |domain| might become dirty.
-bool NET_EXPORT_PRIVATE ParseDomainASCII(base::WStringPiece widestr,
-                                         std::string* domain);
+// Returns empty string on failure.
+std::string NET_EXPORT_PRIVATE ParseDomainASCII(base::WStringPiece widestr);
 
 // Parses |value| as search list (comma-delimited list of domain names) from
-// a registry key and stores it in |out|. Returns true on success. Empty
+// a registry key and stores it in |out|. Returns empty vector on failure. Empty
 // entries (e.g., "chromium.org,,org") terminate the list. Non-ascii hostnames
 // are converted to punycode.
-bool NET_EXPORT_PRIVATE ParseSearchList(const std::wstring& value,
-                                        std::vector<std::string>* out);
+std::vector<std::string> NET_EXPORT_PRIVATE
+ParseSearchList(base::WStringPiece value);
 
 // All relevant settings read from registry and IP Helper. This isolates our
 // logic from system calls and is exposed for unit tests. Keep it an aggregate
@@ -76,6 +74,9 @@ struct NET_EXPORT_PRIVATE DnsSystemSettings {
   DnsSystemSettings();
   ~DnsSystemSettings();
 
+  DnsSystemSettings(DnsSystemSettings&&);
+  DnsSystemSettings& operator=(DnsSystemSettings&&);
+
   // Filled in by GetAdapterAddresses. Note that the alternative
   // GetNetworkParams does not include IPv6 addresses.
   std::unique_ptr<IP_ADAPTER_ADDRESSES, base::FreeDeleter> addresses;
@@ -114,11 +115,10 @@ struct NET_EXPORT_PRIVATE DnsSystemSettings {
   bool have_proxy = false;
 };
 
-// Fills in |dns_config| from |settings|. Exposed for tests. Returns false if a
-// valid config could not be determined.
-bool NET_EXPORT_PRIVATE
-ConvertSettingsToDnsConfig(const DnsSystemSettings& settings,
-                           DnsConfig* dns_config);
+// Fills in |dns_config| from |settings|. Exposed for tests. Returns nullopt if
+// a valid config could not be determined.
+base::Optional<DnsConfig> NET_EXPORT_PRIVATE
+ConvertSettingsToDnsConfig(const DnsSystemSettings& settings);
 
 // Service for reading and watching Windows system DNS settings. This object is
 // not thread-safe and methods may perform blocking I/O so methods must be

net/dns/dns_config_service_win_unittest.cc

@@ -4,11 +4,16 @@
 
 #include "net/dns/dns_config_service_win.h"
 
+#include <string>
+#include <vector>
+
 #include "base/check.h"
 #include "base/memory/free_deleter.h"
+#include "base/optional.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
 #include "net/dns/public/dns_protocol.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -18,33 +23,22 @@ namespace {
 TEST(DnsConfigServiceWinTest, ParseSearchList) {
   const struct TestCase {
     const wchar_t* input;
-    const char* output[4];  // NULL-terminated, empty if expected false
+    std::vector<std::string> expected;
   } cases[] = {
-      {L"chromium.org", {"chromium.org", nullptr}},
-      {L"chromium.org,org", {"chromium.org", "org", nullptr}},
+      {L"chromium.org", {"chromium.org"}},
+      {L"chromium.org,org", {"chromium.org", "org"}},
       // Empty suffixes terminate the list
-      {L"crbug.com,com,,org", {"crbug.com", "com", nullptr}},
+      {L"crbug.com,com,,org", {"crbug.com", "com"}},
       // IDN are converted to punycode
       {L"\u017c\xf3\u0142ta.pi\u0119\u015b\u0107.pl,pl",
-       {"xn--ta-4ja03asj.xn--pi-wla5e0q.pl", "pl", nullptr}},
+       {"xn--ta-4ja03asj.xn--pi-wla5e0q.pl", "pl"}},
       // Empty search list is invalid
-      {L"", {nullptr}},
-      {L",,", {nullptr}},
+      {L"", {}},
+      {L",,", {}},
   };
 
   for (const auto& t : cases) {
-    std::vector<std::string> actual_output, expected_output;
-    actual_output.push_back("UNSET");
-    for (const char* const* output = t.output; *output; ++output) {
-      expected_output.push_back(*output);
-    }
-    bool result = internal::ParseSearchList(t.input, &actual_output);
-    if (!expected_output.empty()) {
-      EXPECT_TRUE(result);
-      EXPECT_EQ(expected_output, actual_output);
-    } else {
-      EXPECT_FALSE(result) << "Unexpected parse success on " << t.input;
-    }
+    EXPECT_EQ(internal::ParseSearchList(t.input), t.expected);
   }
 }
 
@@ -183,14 +177,13 @@ TEST(DnsConfigServiceWinTest, ConvertAdapterAddresses) {
       expected_nameservers.push_back(IPEndPoint(ip, port));
     }
 
-    DnsConfig config;
-    bool success = internal::ConvertSettingsToDnsConfig(settings, &config);
+    base::Optional<DnsConfig> config =
+        internal::ConvertSettingsToDnsConfig(settings);
     bool expected_success = !expected_nameservers.empty();
-    EXPECT_EQ(expected_success, success);
-    EXPECT_EQ(expected_nameservers, config.nameservers);
-    if (success) {
-      ASSERT_EQ(1u, config.search.size());
-      EXPECT_EQ(t.expected_suffix, config.search[0]);
+    EXPECT_EQ(expected_success, config.has_value());
+    if (config.has_value()) {
+      EXPECT_EQ(expected_nameservers, config->nameservers);
+      EXPECT_THAT(config->search, testing::ElementsAre(t.expected_suffix));
     }
   }
 }
@@ -211,7 +204,7 @@ TEST(DnsConfigServiceWinTest, ConvertSuffixSearch) {
       internal::DnsSystemSettings::DevolutionSetting dnscache_devolution;
       internal::DnsSystemSettings::DevolutionSetting tcpip_devolution;
     } input_settings;
-    std::string expected_search[5];
+    std::vector<std::string> expected_search;
   } cases[] = {
       {
           // Policy SearchList override.
@@ -389,13 +382,10 @@ TEST(DnsConfigServiceWinTest, ConvertSuffixSearch) {
     settings.dnscache_devolution = t.input_settings.dnscache_devolution;
     settings.tcpip_devolution = t.input_settings.tcpip_devolution;
 
-    DnsConfig config;
-    EXPECT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config));
-    std::vector<std::string> expected_search;
-    for (size_t j = 0; !t.expected_search[j].empty(); ++j) {
-      expected_search.push_back(t.expected_search[j]);
-    }
-    EXPECT_EQ(expected_search, config.search);
+    EXPECT_THAT(
+        internal::ConvertSettingsToDnsConfig(settings),
+        testing::Optional(testing::Field(
+            &DnsConfig::search, testing::ElementsAreArray(t.expected_search))));
   }
 }
 
@@ -416,9 +406,10 @@ TEST(DnsConfigServiceWinTest, AppendToMultiLabelName) {
     internal::DnsSystemSettings settings;
     settings.addresses = CreateAdapterAddresses(infos);
     settings.append_to_multi_label_name = t.input;
-    DnsConfig config;
-    EXPECT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config));
-    EXPECT_EQ(t.expected_output, config.append_to_multi_label_name);
+    EXPECT_THAT(
+        internal::ConvertSettingsToDnsConfig(settings),
+        testing::Optional(testing::Field(&DnsConfig::append_to_multi_label_name,
+                                         testing::Eq(t.expected_output))));
   }
 }
 
@@ -441,10 +432,11 @@ TEST(DnsConfigServiceWinTest, HaveNRPT) {
     internal::DnsSystemSettings settings;
     settings.addresses = CreateAdapterAddresses(infos);
     settings.have_name_resolution_policy = t.have_nrpt;
-    DnsConfig config;
-    EXPECT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config));
-    EXPECT_EQ(t.unhandled_options, config.unhandled_options);
-    EXPECT_EQ(t.have_nrpt, config.use_local_ipv6);
+    base::Optional<DnsConfig> config =
+        internal::ConvertSettingsToDnsConfig(settings);
+    ASSERT_TRUE(config.has_value());
+    EXPECT_EQ(t.unhandled_options, config->unhandled_options);
+    EXPECT_EQ(t.have_nrpt, config->use_local_ipv6);
   }
 }
 
@@ -467,9 +459,10 @@ TEST(DnsConfigServiceWinTest, HaveProxy) {
     internal::DnsSystemSettings settings;
     settings.addresses = CreateAdapterAddresses(infos);
     settings.have_proxy = t.have_proxy;
-    DnsConfig config;
-    EXPECT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config));
-    EXPECT_EQ(t.unhandled_options, config.unhandled_options);
+    EXPECT_THAT(
+        internal::ConvertSettingsToDnsConfig(settings),
+        testing::Optional(testing::Field(&DnsConfig::unhandled_options,
+                                         testing::Eq(t.unhandled_options))));
   }
 }
 
@@ -483,9 +476,9 @@ TEST(DnsConfigServiceWinTest, UsesVpn) {
 
   internal::DnsSystemSettings settings;
   settings.addresses = CreateAdapterAddresses(infos);
-  DnsConfig config;
-  EXPECT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config));
-  EXPECT_TRUE(config.unhandled_options);
+  EXPECT_THAT(internal::ConvertSettingsToDnsConfig(settings),
+              testing::Optional(testing::Field(&DnsConfig::unhandled_options,
+                                               testing::IsTrue())));
 }
 
 }  // namespace

net/dns/dns_config_watcher_mac.cc

@@ -78,9 +78,7 @@ bool DnsConfigWatcher::Watch(
 }
 
 // static
-bool DnsConfigWatcher::CheckDnsConfig(bool* out_unhandled_options) {
-  DCHECK(out_unhandled_options);
-
+bool DnsConfigWatcher::CheckDnsConfig(bool& out_unhandled_options) {
   if (!GetDnsInfoApi().dns_configuration_copy)
     return false;
   std::unique_ptr<dns_config_t, DnsConfigTDeleter> dns_config(
@@ -100,7 +98,7 @@ bool DnsConfigWatcher::CheckDnsConfig(bool* out_unhandled_options) {
     ++num_resolvers;
   }
 
-  *out_unhandled_options = num_resolvers > 1;
+  out_unhandled_options = num_resolvers > 1;
   return true;
 }
 

net/dns/dns_config_watcher_mac.h

@@ -17,7 +17,7 @@ class DnsConfigWatcher {
   bool Watch(const base::RepeatingCallback<void(bool succeeded)>& callback);
 
   // Returns false iff a valid config could not be determined.
-  static bool CheckDnsConfig(bool* out_unhandled_options);
+  static bool CheckDnsConfig(bool& out_unhandled_options);
 
  private:
   NotifyWatcherMac watcher_;

net/dns/dns_parse_domain_ascii_win_fuzzer.cc

@@ -18,9 +18,9 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
 
   base::StringPiece16 widestr(
       reinterpret_cast<const base::StringPiece16::value_type*>(data), size / 2);
-  std::string result;
+  std::string result = net::internal::ParseDomainASCII(widestr);
 
-  if (net::internal::ParseDomainASCII(widestr, &result))
+  if (!result.empty())
     // Call base::ToLowerASCII to get some additional code coverage signal.
     result = base::ToLowerASCII(result);