Overhaul error handling

13 files changed, 242 insertions, 133 deletions

diff --git a/windows/windns/src/windns/configmanager.cpp b/windows/windns/src/windns/configmanager.cpp
index c9fdde1142..498e58a381 100644
--- a/windows/windns/src/windns/configmanager.cpp
+++ b/windows/windns/src/windns/configmanager.cpp
@@ -8,10 +8,10 @@
 ConfigManager::ConfigManager
 (
 	const std::vector<std::wstring> &servers,
-	const ConfigSinkInfo &configSinkInfo
+	IClientSinkProxy *clientSinkProxy
 )
 	: m_servers(servers)
-	, m_configSinkInfo(configSinkInfo)
+	, m_clientSinkProxy(clientSinkProxy)
 {
 }
 
@@ -31,12 +31,6 @@ void ConfigManager::updateServers(const std::vector<std::wstring> &servers)
 	m_servers = servers;
 }
 
-void ConfigManager::updateConfigSink(const ConfigSinkInfo &configSinkInfo)
-{
-	XTRACE(L"Updating config sink");
-	m_configSinkInfo = configSinkInfo;
-}
-
 const std::vector<std::wstring> &ConfigManager::getServers() const
 {
 	return m_servers;
@@ -135,5 +129,5 @@ void ConfigManager::exportConfigs()
 
 	auto data = s.blob();
 
-	m_configSinkInfo.sink(&data[0], static_cast<uint32_t>(data.size()), m_configSinkInfo.context);
+	m_clientSinkProxy->config(&data[0], static_cast<uint32_t>(data.size()));
 }
diff --git a/windows/windns/src/windns/configmanager.h b/windows/windns/src/windns/configmanager.h
index 157f148901..c6f14f7ea9 100644
--- a/windows/windns/src/windns/configmanager.h
+++ b/windows/windns/src/windns/configmanager.h
@@ -1,7 +1,7 @@
 #pragma once
 
 #include "interfaceconfig.h"
-#include "clientsinkinfo.h"
+#include "iclientsinkproxy.h"
 #include <map>
 #include <string>
 #include <mutex>
@@ -50,7 +50,7 @@ public:
 	ConfigManager
 	(
 		const std::vector<std::wstring> &servers,
-		const ConfigSinkInfo &configSinkInfo
+		IClientSinkProxy *clientSinkProxy
 	);
 
 	//
@@ -61,16 +61,11 @@ public:
 	void unlock();
 
 	//
-	// Notify the ConfigManager that servers used when overriding DNS settings have changed.
+	// Establish the set of servers to use when overriding DNS settings.
 	//
 	void updateServers(const std::vector<std::wstring> &servers);
 
 	//
-	// Update the callback used for persisting settings.
-	//
-	void updateConfigSink(const ConfigSinkInfo &configSinkInfo);
-
-	//
 	// Get the current set of servers used for overriding DNS settings.
 	//
 	const std::vector<std::wstring> &getServers() const;
@@ -96,7 +91,7 @@ private:
 	std::mutex m_mutex;
 
 	std::vector<std::wstring> m_servers;
-	ConfigSinkInfo m_configSinkInfo;
+	IClientSinkProxy *m_clientSinkProxy;
 
 	//
 	// Organize configs based on their system assigned index.
diff --git a/windows/windns/src/windns/confineoperation.cpp b/windows/windns/src/windns/confineoperation.cpp
new file mode 100644
index 0000000000..5c16de71bf
--- /dev/null
+++ b/windows/windns/src/windns/confineoperation.cpp
@@ -0,0 +1,98 @@
+#include "stdafx.h"
+#include "confineoperation.h"
+#include "netsh.h"
+
+bool ConfineOperation
+(
+	const char *literalOperation,
+	std::function<void(const char *, const char **, uint32_t)> errorCallback,
+	std::function<void()> operation
+)
+{
+	try
+	{
+		operation();
+		return true;
+	}
+	catch (NetShError &err)
+	{
+		auto raw = CreateRawStringArray(err.details());
+
+		const char **details = reinterpret_cast<const char **>(&raw[0]);
+		uint32_t numDetails = static_cast<uint32_t>(err.details().size());
+
+		if (0 == numDetails)
+		{
+			details = nullptr;
+		}
+
+		const auto what = std::string(literalOperation).append(": ").append(err.what());
+
+		errorCallback(what.c_str(), details, numDetails);
+
+		return false;
+	}
+	catch (std::exception &err)
+	{
+		const auto what = std::string(literalOperation).append(": ").append(err.what());
+
+		errorCallback(what.c_str(), nullptr, 0);
+
+		return false;
+	}
+	catch (...)
+	{
+		const auto what = std::string(literalOperation).append(": Unspecified failure");
+
+		errorCallback(what.c_str(), nullptr, 0);
+
+		return false;
+	}
+}
+
+std::vector<uint8_t> CreateRawStringArray(const std::vector<std::string> &arr)
+{
+	//
+	// Return a buffer containing a nullptr if there are no items in the array.
+	// This enables clients of this function to address the pointer table.
+	//
+
+	if (arr.empty())
+	{
+		return std::vector<uint8_t>(sizeof(char *), 0);
+	}
+
+	//
+	// Determine total size needed.
+	//
+
+	size_t bufferSize = 0;
+
+	for (const auto &str : arr)
+	{
+		bufferSize += sizeof(char *);
+		bufferSize += (str.size() + 1);
+	}
+
+	//
+	// Copy strings and populate pointer table.
+	//
+
+	std::vector<uint8_t> buffer(bufferSize, 0);
+
+	char **pointerTable = reinterpret_cast<char**>(&buffer[0]);
+	char *data = reinterpret_cast<char*>(&buffer[0] + (sizeof(char*) * arr.size()));
+
+	for (const auto &str : arr)
+	{
+		const auto fullStringSize = str.size() + 1;
+
+		*pointerTable = data;
+		memcpy(data, str.c_str(), fullStringSize);
+
+		++pointerTable;
+		data += fullStringSize;
+	}
+
+	return buffer;
+}
diff --git a/windows/windns/src/windns/confineoperation.h b/windows/windns/src/windns/confineoperation.h
new file mode 100644
index 0000000000..21fe7dd996
--- /dev/null
+++ b/windows/windns/src/windns/confineoperation.h
@@ -0,0 +1,25 @@
+#pragma once
+
+#include <functional>
+#include <vector>
+#include <string>
+#include <cstdint>
+
+bool ConfineOperation
+(
+	const char *literalOperation,
+	std::function<void(const char *, const char **, uint32_t)> errorCallback,
+	std::function<void()> operation
+);
+
+//
+// The returned buffer looks like this:
+//
+// string pointer 1
+// string pointer 2
+// string pointer n
+// string 1
+// string 2
+// string n
+//
+std::vector<uint8_t> CreateRawStringArray(const std::vector<std::string> &arr);
diff --git a/windows/windns/src/windns/iclientsinkproxy.h b/windows/windns/src/windns/iclientsinkproxy.h
new file mode 100644
index 0000000000..9270a12c50
--- /dev/null
+++ b/windows/windns/src/windns/iclientsinkproxy.h
@@ -0,0 +1,13 @@
+#pragma once
+
+#include <cstdint>
+
+struct IClientSinkProxy
+{
+	virtual ~IClientSinkProxy() = 0
+	{
+	}
+
+	virtual void error(const char *errorMessage, const char **details, uint32_t numDetails) = 0;
+	virtual void config(const void *configData, uint32_t dataLength) = 0;
+};
diff --git a/windows/windns/src/windns/netconfigeventsink.cpp b/windows/windns/src/windns/netconfigeventsink.cpp
index e4ef7ae1ce..5b8d4efd07 100644
--- a/windows/windns/src/windns/netconfigeventsink.cpp
+++ b/windows/windns/src/windns/netconfigeventsink.cpp
@@ -1,34 +1,51 @@
 #include "stdafx.h"
 #include "netconfigeventsink.h"
-#include "windns/netconfighelpers.h"
+#include "netconfighelpers.h"
+#include "netsh.h"
+#include "confineoperation.h"
+#include <functional>
 
 using namespace common;
 
-NetConfigEventSink::NetConfigEventSink(std::shared_ptr<wmi::IConnection> connection, std::shared_ptr<ConfigManager> configManager)
+NetConfigEventSink::NetConfigEventSink
+(
+	std::shared_ptr<wmi::IConnection> connection,
+	std::shared_ptr<ConfigManager> configManager,
+	IClientSinkProxy *clientSinkProxy
+)
 	: m_connection(connection)
 	, m_configManager(configManager)
+	, m_clientSinkProxy(clientSinkProxy)
 {
 }
 
 void NetConfigEventSink::update(CComPtr<IWbemClassObject> previous, CComPtr<IWbemClassObject> target)
 {
-	InterfaceConfig previousConfig(previous);
-	InterfaceConfig targetConfig(target);
-
-	ConfigManager::Mutex mutex(*m_configManager);
+	auto forwardError = [this](const char *errorMessage, const char **details, uint32_t numDetails)
+	{
+		m_clientSinkProxy->error(errorMessage, details, numDetails);
+	};
 
-	//
-	// This is OK because the config manager will reject updates
-	// that set our DNS servers.
-	//
-	if (ConfigManager::UpdateStatus::DnsApproved == m_configManager->updateConfig(previousConfig, targetConfig))
+	ConfineOperation("Process adapter update event", forwardError, [&]()
 	{
-		return;
-	}
+		InterfaceConfig previousConfig(previous);
+		InterfaceConfig targetConfig(target);
+
+		ConfigManager::Mutex mutex(*m_configManager);
+
+		//
+		// This is OK because the config manager will reject updates
+		// that set our DNS servers.
+		//
+		if (ConfigManager::UpdateStatus::DnsApproved == m_configManager->updateConfig(previousConfig, targetConfig))
+		{
+			return;
+		}
 
-	//
-	// The update was initiated from an external source.
-	// Override current settings to enforce our selected DNS servers.
-	//
-	nchelpers::SetDnsServers(targetConfig.interfaceIndex(), m_configManager->getServers());
+		//
+		// The update was initiated from an external source.
+		// Override current settings to enforce our selected DNS servers.
+		//
+		nchelpers::SetDnsServers(targetConfig.interfaceIndex(), m_configManager->getServers());
+	});
 }
diff --git a/windows/windns/src/windns/netconfigeventsink.h b/windows/windns/src/windns/netconfigeventsink.h
index 342278c08c..c3fd9242f7 100644
--- a/windows/windns/src/windns/netconfigeventsink.h
+++ b/windows/windns/src/windns/netconfigeventsink.h
@@ -2,14 +2,16 @@
 
 #include "libcommon/wmi/ieventsink.h"
 #include "libcommon/wmi/iconnection.h"
-#include "windns/configmanager.h"
+#include "configmanager.h"
+#include "iclientsinkproxy.h"
 #include <memory>
 
 class NetConfigEventSink : public common::wmi::IModificationEventSink
 {
 public:
 
-	NetConfigEventSink(std::shared_ptr<common::wmi::IConnection> connection, std::shared_ptr<ConfigManager> configManager);
+	NetConfigEventSink(std::shared_ptr<common::wmi::IConnection> connection,
+		std::shared_ptr<ConfigManager> configManager, IClientSinkProxy *clientSinkProxy);
 
 	void update(CComPtr<IWbemClassObject> previous, CComPtr<IWbemClassObject> target) override;
 
@@ -17,4 +19,6 @@ private:
 
 	std::shared_ptr<common::wmi::IConnection> m_connection;
 	std::shared_ptr<ConfigManager> m_configManager;
+
+	IClientSinkProxy *m_clientSinkProxy;
 };
diff --git a/windows/windns/src/windns/windns.cpp b/windows/windns/src/windns/windns.cpp
index 48ad863dfa..662432495a 100644
--- a/windows/windns/src/windns/windns.cpp
+++ b/windows/windns/src/windns/windns.cpp
@@ -2,9 +2,10 @@
 #include "windns.h"
 #include "windnscontext.h"
 #include "clientsinkinfo.h"
-#include "libcommon/serialization/deserializer.h"
 #include "interfaceconfig.h"
 #include "netconfighelpers.h"
+#include "confineoperation.h"
+#include "libcommon/serialization/deserializer.h"
 #include <vector>
 #include <string>
 
@@ -28,6 +29,14 @@ std::vector<std::wstring> MakeStringArray(const wchar_t **strings, uint32_t numS
 	return v;
 }
 
+void ForwardError(const char *errorMessage, const char **details, uint32_t numDetails)
+{
+	if (nullptr != g_ErrorSink)
+	{
+		g_ErrorSink(errorMessage, details, numDetails, g_ErrorContext);
+	}
+}
+
 } // anonymous namespace
 
 WINDNS_LINKAGE
@@ -46,25 +55,10 @@ WinDns_Initialize(
 	g_ErrorSink = errorSink;
 	g_ErrorContext = errorContext;
 
-	try
+	return ConfineOperation("Initialize", ForwardError, []()
 	{
 		g_Context = new WinDnsContext;
-	}
-	catch (std::exception &err)
-	{
-		if (nullptr != g_ErrorSink)
-		{
-			g_ErrorSink(err.what(), g_ErrorContext);
-		}
-
-		return false;
-	}
-	catch (...)
-	{
-		return false;
-	}
-
-	return true;
+	});
 }
 
 WINDNS_LINKAGE
@@ -101,7 +95,7 @@ WinDns_Set(
 		return false;
 	}
 
-	try
+	return ConfineOperation("Enforce DNS settings", ForwardError, [&]()
 	{
 		ClientSinkInfo sinkInfo;
 
@@ -109,22 +103,7 @@ WinDns_Set(
 		sinkInfo.configSinkInfo = ConfigSinkInfo{ configSink, configContext };
 
 		g_Context->set(MakeStringArray(servers, numServers), sinkInfo);
-	}
-	catch (std::exception &err)
-	{
-		if (nullptr != g_ErrorSink)
-		{
-			g_ErrorSink(err.what(), g_ErrorContext);
-		}
-
-		return false;
-	}
-	catch (...)
-	{
-		return false;
-	}
-
-	return true;
+	});
 }
 
 WINDNS_LINKAGE
@@ -138,25 +117,10 @@ WinDns_Reset(
 		return true;
 	}
 
-	try
+	return ConfineOperation("Reset DNS settings", ForwardError, []()
 	{
 		g_Context->reset();
-	}
-	catch (std::exception &err)
-	{
-		if (nullptr != g_ErrorSink)
-		{
-			g_ErrorSink(err.what(), g_ErrorContext);
-		}
-
-		return false;
-	}
-	catch (...)
-	{
-		return false;
-	}
-
-	return true;
+	});
 }
 
 WINDNS_LINKAGE
@@ -169,7 +133,7 @@ WinDns_Recover(
 {
 	std::vector<InterfaceConfig> configs;
 
-	try
+	const auto status = ConfineOperation("Deserialize recovery data", ForwardError, [&]()
 	{
 		common::serialization::Deserializer d(reinterpret_cast<const uint8_t *>(configData), dataLength);
 
@@ -177,7 +141,7 @@ WinDns_Recover(
 
 		if (numConfigs > 50)
 		{
-			return false;
+			throw std::runtime_error("Too many configuration entries");
 		}
 
 		configs.reserve(numConfigs);
@@ -186,48 +150,27 @@ WinDns_Recover(
 		{
 			configs.emplace_back(InterfaceConfig(d));
 		}
-	}
-	catch (std::exception &err)
-	{
-		if (nullptr != g_ErrorSink)
-		{
-			auto msg = std::string("Failed to deserialize recovery data: ").append(err.what());
-
-			g_ErrorSink(msg.c_str(), g_ErrorContext);
-		}
+	});
 
-		return false;
-	}
-	catch (...)
+	if (false == status)
 	{
 		return false;
 	}
 
-	if (configs.empty())
-	{
-		return true;
-	}
+	//
+	// Try to restore each config and update 'success' if any update fails.
+	//
 
 	bool success = true;
 
 	for (const auto &config : configs)
 	{
-		try
+		const auto adapterStatus = ConfineOperation("Restore adapter DNS settings", ForwardError, [&config]()
 		{
 			nchelpers::RevertDnsServers(config);
-		}
-		catch (std::exception &err)
-		{
-			if (nullptr != g_ErrorSink)
-			{
-				auto msg = std::string("Failed to restore interface settings: ").append(err.what());
+		});
 
-				g_ErrorSink(msg.c_str(), g_ErrorContext);
-			}
-
-			success = false;
-		}
-		catch (...)
+		if (false == adapterStatus)
 		{
 			success = false;
 		}
diff --git a/windows/windns/src/windns/windns.h b/windows/windns/src/windns/windns.h
index 734719ca80..ab4ec9a539 100644
--- a/windows/windns/src/windns/windns.h
+++ b/windows/windns/src/windns/windns.h
@@ -17,7 +17,7 @@
 // Functions
 ///////////////////////////////////////////////////////////////////////////////
 
-typedef void (WINDNS_API *WinDnsErrorSink)(const char *errorMessage, void *context);
+typedef void (WINDNS_API *WinDnsErrorSink)(const char *errorMessage, const char **details, uint32_t numDetails, void *context);
 typedef void (WINDNS_API *WinDnsConfigSink)(const void *configData, uint32_t dataLength, void *context);
 
 //
diff --git a/windows/windns/src/windns/windns.vcxproj b/windows/windns/src/windns/windns.vcxproj
index a5fb31bb55..8b8bbb124a 100644
--- a/windows/windns/src/windns/windns.vcxproj
+++ b/windows/windns/src/windns/windns.vcxproj
@@ -177,6 +177,8 @@
   <ItemGroup>
     <ClInclude Include="clientsinkinfo.h" />
     <ClInclude Include="configmanager.h" />
+    <ClInclude Include="confineoperation.h" />
+    <ClInclude Include="iclientsinkproxy.h" />
     <ClInclude Include="interfaceconfig.h" />
     <ClInclude Include="netconfigeventsink.h" />
     <ClInclude Include="netconfighelpers.h" />
@@ -189,6 +191,7 @@
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="configmanager.cpp" />
+    <ClCompile Include="confineoperation.cpp" />
     <ClCompile Include="dllmain.cpp" />
     <ClCompile Include="interfaceconfig.cpp" />
     <ClCompile Include="netconfigeventsink.cpp" />
diff --git a/windows/windns/src/windns/windns.vcxproj.filters b/windows/windns/src/windns/windns.vcxproj.filters
index 3c10de37d9..57e9eeaef0 100644
--- a/windows/windns/src/windns/windns.vcxproj.filters
+++ b/windows/windns/src/windns/windns.vcxproj.filters
@@ -12,6 +12,8 @@
     <ClInclude Include="netsh.h" />
     <ClInclude Include="interfaceconfig.h" />
     <ClInclude Include="types.h" />
+    <ClInclude Include="iclientsinkproxy.h" />
+    <ClInclude Include="confineoperation.h" />
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="dllmain.cpp" />
@@ -23,6 +25,7 @@
     <ClCompile Include="netconfighelpers.cpp" />
     <ClCompile Include="netsh.cpp" />
     <ClCompile Include="interfaceconfig.cpp" />
+    <ClCompile Include="confineoperation.cpp" />
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="windns.rc" />
diff --git a/windows/windns/src/windns/windnscontext.cpp b/windows/windns/src/windns/windnscontext.cpp
index 17265f0b65..0dfdaf2359 100644
--- a/windows/windns/src/windns/windnscontext.cpp
+++ b/windows/windns/src/windns/windnscontext.cpp
@@ -17,13 +17,6 @@ WinDnsContext::~WinDnsContext()
 	{
 		reset();
 	}
-	catch (std::exception &err)
-	{
-		if (nullptr != m_sinkInfo.errorSinkInfo.sink)
-		{
-			m_sinkInfo.errorSinkInfo.sink(err.what(), m_sinkInfo.errorSinkInfo.context);
-		}
-	}
 	catch (...)
 	{
 	}
@@ -35,13 +28,13 @@ void WinDnsContext::set(const std::vector<std::wstring> &servers, const ClientSi
 
 	if (nullptr == m_notification)
 	{
-		m_configManager = std::make_shared<ConfigManager>(servers, m_sinkInfo.configSinkInfo);
+		m_configManager = std::make_shared<ConfigManager>(servers, this);
 
 		//
 		// Register interface configuration monitoring.
 		//
 
-		auto eventSink = std::make_shared<NetConfigEventSink>(m_connection, m_configManager);
+		auto eventSink = std::make_shared<NetConfigEventSink>(m_connection, m_configManager, this);
 		auto eventDispatcher = CComPtr<wmi::IEventDispatcher>(new wmi::ModificationEventDispatcher(eventSink));
 
 		m_notification = std::make_unique<wmi::Notification>(m_connection, eventDispatcher);
@@ -60,7 +53,6 @@ void WinDnsContext::set(const std::vector<std::wstring> &servers, const ClientSi
 		ConfigManager::Mutex mutex(*m_configManager);
 
 		m_configManager->updateServers(servers);
-		m_configManager->updateConfigSink(m_sinkInfo.configSinkInfo);
 	}
 
 	//
@@ -96,3 +88,21 @@ void WinDnsContext::reset()
 		return true;
 	});
 }
+
+// IClientSinkProxy
+void WinDnsContext::error(const char *errorMessage, const char **details, uint32_t numDetails)
+{
+	if (nullptr != m_sinkInfo.errorSinkInfo.sink)
+	{
+		m_sinkInfo.errorSinkInfo.sink(errorMessage, details, numDetails, m_sinkInfo.errorSinkInfo.context);
+	}
+}
+
+// IClientSinkProxy
+void WinDnsContext::config(const void *configData, uint32_t dataLength)
+{
+	if (nullptr != m_sinkInfo.configSinkInfo.sink)
+	{
+		m_sinkInfo.configSinkInfo.sink(configData, dataLength, m_sinkInfo.configSinkInfo.context);
+	}
+}
diff --git a/windows/windns/src/windns/windnscontext.h b/windows/windns/src/windns/windnscontext.h
index c514153edd..c83ec6e482 100644
--- a/windows/windns/src/windns/windnscontext.h
+++ b/windows/windns/src/windns/windnscontext.h
@@ -5,11 +5,12 @@
 #include "libcommon/wmi/notification.h"
 #include "configmanager.h"
 #include "clientsinkinfo.h"
+#include "iclientsinkproxy.h"
 #include <vector>
 #include <string>
 #include <memory>
 
-class WinDnsContext
+class WinDnsContext : public IClientSinkProxy
 {
 public:
 
@@ -19,6 +20,9 @@ public:
 	void set(const std::vector<std::wstring> &servers, const ClientSinkInfo &sinkInfo);
 	void reset();
 
+	void IClientSinkProxy::error(const char *errorMessage, const char **details, uint32_t numDetails) override;
+	void IClientSinkProxy::config(const void *configData, uint32_t dataLength) override;
+
 private:
 
 	std::shared_ptr<common::wmi::Connection> m_connection;