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.
This commit is contained in:
Jason A. Donenfeld 2018-11-03 03:40:23 +01:00
parent 6c737545aa
commit 2fdbe1c492
3 changed files with 33 additions and 51 deletions

View File

@ -11,6 +11,8 @@ class ErrorPresenter {
// TunnelManagementError // TunnelManagementError
case TunnelManagementError.tunnelAlreadyExistsWithThatName: case TunnelManagementError.tunnelAlreadyExistsWithThatName:
return ("Name already exists", "A tunnel with that name already exists") 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: case TunnelManagementError.vpnSystemErrorOnAddTunnel:
return ("Unable to create tunnel", "Internal error") return ("Unable to create tunnel", "Internal error")
case TunnelManagementError.vpnSystemErrorOnModifyTunnel: case TunnelManagementError.vpnSystemErrorOnModifyTunnel:

View File

@ -313,8 +313,8 @@ extension TunnelsListTableViewController: TunnelsManagerDelegate {
tableView.reloadRows(at: [IndexPath(row: index, section: 0)], with: .automatic) tableView.reloadRows(at: [IndexPath(row: index, section: 0)], with: .automatic)
} }
func tunnelsChanged() { func tunnelMoved(at oldIndex: Int, to newIndex: Int) {
tableView.reloadData() tableView.moveRow(at: IndexPath(row: oldIndex, section: 0), to: IndexPath(row: newIndex, section: 0))
} }
func tunnelRemoved(at index: Int) { func tunnelRemoved(at index: Int) {

View File

@ -8,7 +8,7 @@ import os.log
protocol TunnelsManagerDelegate: class { protocol TunnelsManagerDelegate: class {
func tunnelAdded(at: Int) func tunnelAdded(at: Int)
func tunnelModified(at: Int) func tunnelModified(at: Int)
func tunnelsChanged() func tunnelMoved(at oldIndex: Int, to newIndex: Int)
func tunnelRemoved(at: Int) func tunnelRemoved(at: Int)
} }
@ -23,6 +23,7 @@ enum TunnelActivationError: Error {
enum TunnelManagementError: Error { enum TunnelManagementError: Error {
case tunnelAlreadyExistsWithThatName case tunnelAlreadyExistsWithThatName
case tunnelInvalidName
case vpnSystemErrorOnAddTunnel case vpnSystemErrorOnAddTunnel
case vpnSystemErrorOnModifyTunnel case vpnSystemErrorOnModifyTunnel
case vpnSystemErrorOnRemoveTunnel case vpnSystemErrorOnRemoveTunnel
@ -43,12 +44,10 @@ class TunnelsManager {
init(tunnelProviders: [NETunnelProviderManager]) { init(tunnelProviders: [NETunnelProviderManager]) {
var tunnelNames: Set<String> = [] var tunnelNames: Set<String> = []
var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0, index: 0) } var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0) }
tunnels.sort { $0.name < $1.name } tunnels.sort { $0.name < $1.name }
var currentTunnel: TunnelContainer? = nil var currentTunnel: TunnelContainer? = nil
for i in 0 ..< tunnels.count { for tunnel in tunnels {
let tunnel = tunnels[i]
tunnel.index = i
tunnelNames.insert(tunnel.name) tunnelNames.insert(tunnel.name)
if (tunnel.status != .inactive) { if (tunnel.status != .inactive) {
currentTunnel = tunnel currentTunnel = tunnel
@ -75,17 +74,12 @@ class TunnelsManager {
return tunnelNames.contains(name) 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) { func add(tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelContainer?, TunnelManagementError?) -> Void) {
let tunnelName = tunnelConfiguration.interface.name let tunnelName = tunnelConfiguration.interface.name
assert(!tunnelName.isEmpty) if tunnelName.isEmpty {
completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName)
return
}
guard (!containsTunnel(named: tunnelName)) else { guard (!containsTunnel(named: tunnelName)) else {
completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName) completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName)
@ -106,14 +100,11 @@ class TunnelsManager {
return return
} }
if let s = self { if let s = self {
let index = s.insertionIndexFor(tunnelName: tunnelName) let tunnel = TunnelContainer(tunnel: tunnelProviderManager)
let tunnel = TunnelContainer(tunnel: tunnelProviderManager, index: index) s.tunnels.append(tunnel)
for i in index ..< s.tunnels.count { s.tunnels.sort { $0.name < $1.name }
s.tunnels[i].index = s.tunnels[i].index + 1
}
s.tunnels.insert(tunnel, at: index)
s.tunnelNames.insert(tunnel.name) s.tunnelNames.insert(tunnel.name)
s.delegate?.tunnelAdded(at: index) s.delegate?.tunnelAdded(at: s.tunnels.firstIndex(of: tunnel)!)
completionHandler(tunnel, nil) completionHandler(tunnel, nil)
} }
} }
@ -139,7 +130,10 @@ class TunnelsManager {
func modify(tunnel: TunnelContainer, with tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelManagementError?) -> Void) { func modify(tunnel: TunnelContainer, with tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelManagementError?) -> Void) {
let tunnelName = tunnelConfiguration.interface.name let tunnelName = tunnelConfiguration.interface.name
assert(!tunnelName.isEmpty) if tunnelName.isEmpty {
completionHandler(TunnelManagementError.tunnelAlreadyExistsWithThatName)
return
}
isModifyingTunnel = true isModifyingTunnel = true
@ -167,22 +161,14 @@ class TunnelsManager {
} }
if let s = self { if let s = self {
if (isNameChanged) { if (isNameChanged) {
s.tunnels.remove(at: tunnel.index) let oldIndex = s.tunnels.firstIndex(of: tunnel)!
s.tunnelNames.remove(oldName!) 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.tunnelNames.insert(tunnel.name)
s.delegate?.tunnelsChanged() s.tunnels.sort { $0.name < $1.name }
} else { let newIndex = s.tunnels.firstIndex(of: tunnel)!
s.delegate?.tunnelModified(at: tunnel.index) 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) { 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 // 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) { func remove(tunnel: TunnelContainer, completionHandler: @escaping (TunnelManagementError?) -> Void) {
let tunnelProviderManager = tunnel.tunnelProvider let tunnelProviderManager = tunnel.tunnelProvider
let tunnelIndex = tunnel.index
let tunnelName = tunnel.name
isDeletingTunnel = true isDeletingTunnel = true
@ -209,12 +193,10 @@ class TunnelsManager {
return return
} }
if let s = self { if let s = self {
for i in ((tunnelIndex + 1) ..< s.tunnels.count) { let index = s.tunnels.firstIndex(of: tunnel)!
s.tunnels[i].index = s.tunnels[i].index - 1 s.tunnels.remove(at: index)
} s.tunnelNames.remove(tunnel.name)
s.tunnels.remove(at: tunnelIndex) s.delegate?.tunnelRemoved(at: index)
s.tunnelNames.remove(tunnelName)
s.delegate?.tunnelRemoved(at: tunnelIndex)
} }
completionHandler(nil) completionHandler(nil)
} }
@ -246,7 +228,7 @@ class TunnelsManager {
completionHandler(TunnelActivationError.attemptingDeactivationWhenTunnelIsInactive) completionHandler(TunnelActivationError.attemptingDeactivationWhenTunnelIsInactive)
return return
} }
assert(tunnel.index == currentTunnel!.index) assert(tunnel == currentTunnel!)
tunnel.startDeactivation() tunnel.startDeactivation()
} }
@ -293,17 +275,15 @@ class TunnelContainer: NSObject {
@objc dynamic var status: TunnelStatus @objc dynamic var status: TunnelStatus
fileprivate let tunnelProvider: NETunnelProviderManager fileprivate let tunnelProvider: NETunnelProviderManager
fileprivate var index: Int
fileprivate var statusObservationToken: AnyObject? fileprivate var statusObservationToken: AnyObject?
private var dnsResolver: DNSResolver? = nil private var dnsResolver: DNSResolver? = nil
init(tunnel: NETunnelProviderManager, index: Int) { init(tunnel: NETunnelProviderManager) {
self.name = tunnel.localizedDescription ?? "Unnamed" self.name = tunnel.localizedDescription ?? "Unnamed"
let status = TunnelStatus(from: tunnel.connection.status) let status = TunnelStatus(from: tunnel.connection.status)
self.status = status self.status = status
self.tunnelProvider = tunnel self.tunnelProvider = tunnel
self.index = index
super.init() super.init()
if (status != .inactive) { if (status != .inactive) {
startObservingTunnelStatus() startObservingTunnelStatus()