From 68c7fc41309ee44ba57a0487c078f2ab00f81d04 Mon Sep 17 00:00:00 2001
From: Razvan Dimescu <ssaricu@gmail.com>
Date: Wed, 8 Apr 2026 04:33:34 +0300
Subject: [PATCH] refactor: share iter_nameservers helper and reuse resolv.conf
 content
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Post-review simplifications on the stale-backup fix:

- Extract iter_nameservers(&str) helper used by both parse_resolv_conf
  and resolv_conf_has_real_upstream. Eliminates the duplicated
  line-by-line nameserver parsing (findings from reuse review).
- install_linux: reuse the already-read resolv.conf content via
  std::fs::write instead of a second read via std::fs::copy.
- install_macos / install_windows: flatten the conditional eprintln
  pattern — always print a blank line, conditionally print the save
  message. Equivalent output, less branching.

Net −12 lines. All 130 tests still pass, clippy clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 src/system_dns.rs | 68 +++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/src/system_dns.rs b/src/system_dns.rs
index 2657488..6c8bef8 100644
--- a/src/system_dns.rs
+++ b/src/system_dns.rs
@@ -214,7 +214,18 @@ fn discover_linux() -> SystemDnsInfo {
     }
 }
 
-/// Parse resolv.conf in a single pass, extracting both the first non-loopback
+/// Yield each `nameserver` address from resolv.conf content. No filtering —
+/// callers decide what counts as a real upstream.
+#[cfg(any(target_os = "linux", test))]
+fn iter_nameservers(content: &str) -> impl Iterator<Item = &str> {
+    content.lines().filter_map(|line| {
+        let mut parts = line.trim().split_whitespace();
+        (parts.next() == Some("nameserver")).then_some(())?;
+        parts.next()
+    })
+}
+
+/// Parse resolv.conf in a single pass, extracting the first non-loopback
 /// nameserver and all search domains.
 #[cfg(target_os = "linux")]
 fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
@@ -222,19 +233,13 @@ fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
         Ok(t) => t,
         Err(_) => return (None, Vec::new()),
     };
-    let mut upstream = None;
+    let upstream = iter_nameservers(&text)
+        .find(|ns| !is_loopback_or_stub(ns))
+        .map(str::to_string);
     let mut search_domains = Vec::new();
     for line in text.lines() {
         let line = line.trim();
-        if line.starts_with("nameserver") {
-            if upstream.is_none() {
-                if let Some(ns) = line.split_whitespace().nth(1) {
-                    if !is_loopback_or_stub(ns) {
-                        upstream = Some(ns.to_string());
-                    }
-                }
-            }
-        } else if line.starts_with("search") || line.starts_with("domain") {
+        if line.starts_with("search") || line.starts_with("domain") {
             for domain in line.split_whitespace().skip(1) {
                 search_domains.push(domain.to_string());
             }
@@ -248,27 +253,14 @@ fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
 /// for a backup.
 #[cfg(any(target_os = "linux", test))]
 fn resolv_conf_is_numa_managed(content: &str) -> bool {
-    if content.contains("Generated by Numa") {
-        return true;
-    }
-    !resolv_conf_has_real_upstream(content)
+    content.contains("Generated by Numa") || !resolv_conf_has_real_upstream(content)
 }
 
 /// True if the resolv.conf content has at least one non-loopback, non-stub
 /// nameserver. An all-loopback resolv.conf is self-referential.
 #[cfg(any(target_os = "linux", test))]
 fn resolv_conf_has_real_upstream(content: &str) -> bool {
-    for line in content.lines() {
-        let line = line.trim();
-        if let Some(rest) = line.strip_prefix("nameserver") {
-            if let Some(ns) = rest.split_whitespace().next() {
-                if !is_loopback_or_stub(ns) {
-                    return true;
-                }
-            }
-        }
-    }
-    false
+    iter_nameservers(content).any(|ns| !is_loopback_or_stub(ns))
 }
 
 /// Query resolvectl for the real upstream DNS server (e.g. VPC resolver on AWS).
@@ -629,10 +621,9 @@ fn install_windows() -> Result<(), String> {
     let needs_reboot = disable_dnscache()?;
     register_autostart();
 
-    if has_useful_existing {
-        eprintln!();
-    } else {
-        eprintln!("\n  Original DNS saved to {}", path.display());
+    eprintln!();
+    if !has_useful_existing {
+        eprintln!("  Original DNS saved to {}", path.display());
     }
     eprintln!("  Run 'numa uninstall' to restore.\n");
     if needs_reboot {
@@ -891,10 +882,9 @@ fn install_macos() -> Result<(), String> {
             .status();
     }
 
-    if has_useful_existing {
-        eprintln!();
-    } else {
-        eprintln!("\n  Original DNS saved to {}", backup_path().display());
+    eprintln!();
+    if !has_useful_existing {
+        eprintln!("  Original DNS saved to {}", backup_path().display());
     }
     eprintln!("  Run 'sudo numa uninstall' to restore.\n");
 
@@ -1253,12 +1243,10 @@ fn install_linux() -> Result<(), String> {
         );
     } else if current_is_numa_managed {
         eprintln!("  warning: /etc/resolv.conf is already numa-managed; no fresh backup written");
-    } else {
-        match std::fs::copy(resolv, &backup) {
-            Ok(_) => eprintln!("  Saved /etc/resolv.conf to {}", backup.display()),
-            Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
-            Err(e) => return Err(format!("failed to backup /etc/resolv.conf: {}", e)),
-        }
+    } else if let Some(content) = current.as_deref() {
+        std::fs::write(&backup, content)
+            .map_err(|e| format!("failed to backup /etc/resolv.conf: {}", e))?;
+        eprintln!("  Saved /etc/resolv.conf to {}", backup.display());
     }
 
     if resolv