From 2fdbe1c49219567124375d33a1f9b4458a8e7472 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Sat, 3 Nov 2018 03:40:23 +0100 Subject: [PATCH] TunnelsManager: get rid of index management No need for premature optimization. There aren't that many tunnels most of the time, and calling sort on a partially sorted array is fast. --- .../WireGuard/UI/iOS/ErrorPresenter.swift | 2 + .../iOS/TunnelsListTableViewController.swift | 8 +- WireGuard/WireGuard/VPN/TunnelsManager.swift | 74 +++++++------------ 3 files changed, 33 insertions(+), 51 deletions(-) diff --git a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift index cb63867..a73c057 100644 --- a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift +++ b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift @@ -11,6 +11,8 @@ class ErrorPresenter { // TunnelManagementError case TunnelManagementError.tunnelAlreadyExistsWithThatName: return ("Name already exists", "A tunnel with that name already exists") + case TunnelManagementError.tunnelInvalidName: + return ("Name already exists", "The tunnel name is invalid") case TunnelManagementError.vpnSystemErrorOnAddTunnel: return ("Unable to create tunnel", "Internal error") case TunnelManagementError.vpnSystemErrorOnModifyTunnel: diff --git a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift index 9515885..1ab7dd7 100644 --- a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift +++ b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift @@ -312,11 +312,11 @@ extension TunnelsListTableViewController: TunnelsManagerDelegate { func tunnelModified(at index: Int) { tableView.reloadRows(at: [IndexPath(row: index, section: 0)], with: .automatic) } - - func tunnelsChanged() { - tableView.reloadData() + + func tunnelMoved(at oldIndex: Int, to newIndex: Int) { + tableView.moveRow(at: IndexPath(row: oldIndex, section: 0), to: IndexPath(row: newIndex, section: 0)) } - + func tunnelRemoved(at index: Int) { tableView.deleteRows(at: [IndexPath(row: index, section: 0)], with: .automatic) } diff --git a/WireGuard/WireGuard/VPN/TunnelsManager.swift b/WireGuard/WireGuard/VPN/TunnelsManager.swift index 4a43755..5a89201 100644 --- a/WireGuard/WireGuard/VPN/TunnelsManager.swift +++ b/WireGuard/WireGuard/VPN/TunnelsManager.swift @@ -8,7 +8,7 @@ import os.log protocol TunnelsManagerDelegate: class { func tunnelAdded(at: Int) func tunnelModified(at: Int) - func tunnelsChanged() + func tunnelMoved(at oldIndex: Int, to newIndex: Int) func tunnelRemoved(at: Int) } @@ -23,6 +23,7 @@ enum TunnelActivationError: Error { enum TunnelManagementError: Error { case tunnelAlreadyExistsWithThatName + case tunnelInvalidName case vpnSystemErrorOnAddTunnel case vpnSystemErrorOnModifyTunnel case vpnSystemErrorOnRemoveTunnel @@ -43,12 +44,10 @@ class TunnelsManager { init(tunnelProviders: [NETunnelProviderManager]) { var tunnelNames: Set = [] - var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0, index: 0) } + var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0) } tunnels.sort { $0.name < $1.name } var currentTunnel: TunnelContainer? = nil - for i in 0 ..< tunnels.count { - let tunnel = tunnels[i] - tunnel.index = i + for tunnel in tunnels { tunnelNames.insert(tunnel.name) if (tunnel.status != .inactive) { currentTunnel = tunnel @@ -75,17 +74,12 @@ class TunnelsManager { return tunnelNames.contains(name) } - private func insertionIndexFor(tunnelName: String) -> Int { - // Wishlist: Use binary search instead - for i in 0 ..< tunnels.count { - if (tunnelName.lexicographicallyPrecedes(tunnels[i].name)) { return i } - } - return tunnels.count - } - func add(tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelContainer?, TunnelManagementError?) -> Void) { let tunnelName = tunnelConfiguration.interface.name - assert(!tunnelName.isEmpty) + if tunnelName.isEmpty { + completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName) + return + } guard (!containsTunnel(named: tunnelName)) else { completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName) @@ -106,14 +100,11 @@ class TunnelsManager { return } if let s = self { - let index = s.insertionIndexFor(tunnelName: tunnelName) - let tunnel = TunnelContainer(tunnel: tunnelProviderManager, index: index) - for i in index ..< s.tunnels.count { - s.tunnels[i].index = s.tunnels[i].index + 1 - } - s.tunnels.insert(tunnel, at: index) + let tunnel = TunnelContainer(tunnel: tunnelProviderManager) + s.tunnels.append(tunnel) + s.tunnels.sort { $0.name < $1.name } s.tunnelNames.insert(tunnel.name) - s.delegate?.tunnelAdded(at: index) + s.delegate?.tunnelAdded(at: s.tunnels.firstIndex(of: tunnel)!) completionHandler(tunnel, nil) } } @@ -139,7 +130,10 @@ class TunnelsManager { func modify(tunnel: TunnelContainer, with tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelManagementError?) -> Void) { let tunnelName = tunnelConfiguration.interface.name - assert(!tunnelName.isEmpty) + if tunnelName.isEmpty { + completionHandler(TunnelManagementError.tunnelAlreadyExistsWithThatName) + return + } isModifyingTunnel = true @@ -167,22 +161,14 @@ class TunnelsManager { } if let s = self { if (isNameChanged) { - s.tunnels.remove(at: tunnel.index) + let oldIndex = s.tunnels.firstIndex(of: tunnel)! s.tunnelNames.remove(oldName!) - for i in tunnel.index ..< s.tunnels.count { - s.tunnels[i].index = s.tunnels[i].index - 1 - } - let index = s.insertionIndexFor(tunnelName: tunnelName) - tunnel.index = index - for i in index ..< s.tunnels.count { - s.tunnels[i].index = s.tunnels[i].index + 1 - } - s.tunnels.insert(tunnel, at: index) s.tunnelNames.insert(tunnel.name) - s.delegate?.tunnelsChanged() - } else { - s.delegate?.tunnelModified(at: tunnel.index) + s.tunnels.sort { $0.name < $1.name } + let newIndex = s.tunnels.firstIndex(of: tunnel)! + s.delegate?.tunnelMoved(at: oldIndex, to: newIndex) } + s.delegate?.tunnelModified(at: s.tunnels.firstIndex(of: tunnel)!) if (tunnel.status == .active || tunnel.status == .activating || tunnel.status == .reasserting) { // Turn off the tunnel, and then turn it back on, so the changes are made effective @@ -196,8 +182,6 @@ class TunnelsManager { func remove(tunnel: TunnelContainer, completionHandler: @escaping (TunnelManagementError?) -> Void) { let tunnelProviderManager = tunnel.tunnelProvider - let tunnelIndex = tunnel.index - let tunnelName = tunnel.name isDeletingTunnel = true @@ -209,12 +193,10 @@ class TunnelsManager { return } if let s = self { - for i in ((tunnelIndex + 1) ..< s.tunnels.count) { - s.tunnels[i].index = s.tunnels[i].index - 1 - } - s.tunnels.remove(at: tunnelIndex) - s.tunnelNames.remove(tunnelName) - s.delegate?.tunnelRemoved(at: tunnelIndex) + let index = s.tunnels.firstIndex(of: tunnel)! + s.tunnels.remove(at: index) + s.tunnelNames.remove(tunnel.name) + s.delegate?.tunnelRemoved(at: index) } completionHandler(nil) } @@ -246,7 +228,7 @@ class TunnelsManager { completionHandler(TunnelActivationError.attemptingDeactivationWhenTunnelIsInactive) return } - assert(tunnel.index == currentTunnel!.index) + assert(tunnel == currentTunnel!) tunnel.startDeactivation() } @@ -293,17 +275,15 @@ class TunnelContainer: NSObject { @objc dynamic var status: TunnelStatus fileprivate let tunnelProvider: NETunnelProviderManager - fileprivate var index: Int fileprivate var statusObservationToken: AnyObject? private var dnsResolver: DNSResolver? = nil - init(tunnel: NETunnelProviderManager, index: Int) { + init(tunnel: NETunnelProviderManager) { self.name = tunnel.localizedDescription ?? "Unnamed" let status = TunnelStatus(from: tunnel.connection.status) self.status = status self.tunnelProvider = tunnel - self.index = index super.init() if (status != .inactive) { startObservingTunnelStatus()