From de7ec99ad93edbb30ba743ed63af240dbac61a17 Mon Sep 17 00:00:00 2001 From: Eva Pace Date: Fri, 2 Jun 2023 03:42:54 -0300 Subject: [PATCH] Optimize /etc/hosts writes (#259) * hostsfile: change internal map from hash to btree This change makes the innernet section of /etc/hosts always ordered and deterministic. We can take advantage of that to avoid writes, that will be done in another commit. * hostsfile: reduce number of writes if content hasn't changed * hostsfile: return bool to inform if file has been written This commit also makes the logs print accordingly to the new behavior. * hostsfile: remove has_content_changed in favor of comparing old and new sections * hostsfile: print the correct hosts path in log message * hostsfile: remove unnecessary intermediate variable --- client/src/main.rs | 15 +++++++---- hostsfile/src/lib.rs | 64 ++++++++++++++++++++++++++++---------------- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/client/src/main.rs b/client/src/main.rs index 86095ee..c1b3155 100644 --- a/client/src/main.rs +++ b/client/src/main.rs @@ -279,8 +279,6 @@ fn update_hosts_file( hosts_path: PathBuf, peers: &[Peer], ) -> Result<(), WrappedIoError> { - log::info!("updating {} with the latest peers.", "/etc/hosts".yellow()); - let mut hosts_builder = HostsBuilder::new(format!("innernet {interface}")); for peer in peers { hosts_builder.add_hostname( @@ -288,9 +286,16 @@ fn update_hosts_file( &format!("{}.{}.wg", peer.contents.name, interface), ); } - if let Err(e) = hosts_builder.write_to(&hosts_path).with_path(hosts_path) { - log::warn!("failed to update hosts ({})", e); - } + match hosts_builder.write_to(&hosts_path).with_path(&hosts_path) { + Ok(has_written) if has_written => { + log::info!( + "updated {} with the latest peers.", + hosts_path.to_string_lossy().yellow() + ) + }, + Ok(_) => {}, + Err(e) => log::warn!("failed to update hosts ({})", e), + }; Ok(()) } diff --git a/hostsfile/src/lib.rs b/hostsfile/src/lib.rs index 7c055e5..b6ba2e1 100644 --- a/hostsfile/src/lib.rs +++ b/hostsfile/src/lib.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::BTreeMap, fmt, fs::OpenOptions, io::{self, BufRead, BufReader, ErrorKind, Write}, @@ -81,7 +81,7 @@ impl std::error::Error for Error { /// ``` pub struct HostsBuilder { tag: String, - hostname_map: HashMap>, + hostname_map: BTreeMap>, } impl HostsBuilder { @@ -90,7 +90,7 @@ impl HostsBuilder { pub fn new>(tag: S) -> Self { Self { tag: tag.into(), - hostname_map: HashMap::new(), + hostname_map: BTreeMap::new(), } } @@ -116,7 +116,8 @@ impl HostsBuilder { /// Inserts a new section to the system's default hosts file. If there is a section with the /// same tag name already, it will be replaced with the new list instead. - pub fn write(&self) -> io::Result<()> { + /// Returns true if the hosts file has changed. + pub fn write(&self) -> io::Result { self.write_to(Self::default_path()?) } @@ -178,7 +179,9 @@ impl HostsBuilder { /// /// On Windows, the format of one hostname per line will be used, all other systems will use /// the same format as Unix and Unix-like systems (i.e. allow multiple hostnames per line). - pub fn write_to>(&self, hosts_path: P) -> io::Result<()> { + /// + /// Returns true if the hosts file has changed. + pub fn write_to>(&self, hosts_path: P) -> io::Result { let hosts_path = hosts_path.as_ref(); if hosts_path.is_dir() { // TODO(jake): use io::ErrorKind::IsADirectory when it's stable. @@ -206,9 +209,31 @@ impl HostsBuilder { let begin = lines.iter().position(|line| line.trim() == begin_marker); let end = lines.iter().position(|line| line.trim() == end_marker); + let mut lines_to_insert = vec![]; + if !self.hostname_map.is_empty() { + lines_to_insert.push(begin_marker); + for (ip, hostnames) in &self.hostname_map { + if cfg!(windows) { + // windows only allows one hostname per line + for hostname in hostnames { + lines_to_insert.push(format!("{ip} {hostname}")); + } + } else { + // assume the same format as Unix + lines_to_insert.push(format!("{} {}", ip, hostnames.join(" "))); + } + } + lines_to_insert.push(end_marker); + } + let insert = match (begin, end) { (Some(begin), Some(end)) => { - lines.drain(begin..end + 1); + let old_section: Vec = lines.drain(begin..end + 1).collect(); + + if old_section == lines_to_insert { + return Ok(false); + } + begin }, (None, None) => { @@ -233,21 +258,12 @@ impl HostsBuilder { for line in &lines[..insert] { writeln!(s, "{line}")?; } - if !self.hostname_map.is_empty() { - writeln!(s, "{begin_marker}")?; - for (ip, hostnames) in &self.hostname_map { - if cfg!(windows) { - // windows only allows one hostname per line - for hostname in hostnames { - writeln!(s, "{ip} {hostname}")?; - } - } else { - // assume the same format as Unix - writeln!(s, "{} {}", ip, hostnames.join(" "))?; - } - } - writeln!(s, "{end_marker}")?; + + // Append hostnames_map section + for line in lines_to_insert { + writeln!(s, "{line}")?; } + for line in &lines[insert..] { writeln!(s, "{line}")?; } @@ -260,8 +276,9 @@ impl HostsBuilder { _ => { log::debug!("wrote hosts file with the write-and-swap strategy"); }, - } - Ok(()) + }; + + Ok(true) } fn write_and_swap(temp_path: &Path, hosts_path: &Path, contents: &[u8]) -> io::Result<()> { @@ -314,7 +331,8 @@ mod tests { temp_file.write_all(b"preexisting\ncontent").unwrap(); let mut builder = HostsBuilder::new("foo"); builder.add_hostname([1, 1, 1, 1].into(), "whatever"); - builder.write_to(&temp_path).unwrap(); + assert!(builder.write_to(&temp_path).unwrap()); + assert!(!builder.write_to(&temp_path).unwrap()); let contents = std::fs::read_to_string(&temp_path).unwrap(); println!("contents: {contents}");