cleanup FIXMEs from proto refactor

CHANGELOG.md

@@ -19,8 +19,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - Internal API changes to `client` calling into `proto` for actual implementations
 - Large refactoring of internal APIs to more cleanly support \*ring\* and OpenSSL features (@briansmith)
 - `ClientHandle::send` moved to `trust_dns_proto::DnsHandle::send` (internal API)
-- Many interfaces moved from `ClientStreamHandle` to `trust_dns_proto::DnsStreamHandle`
+- Many interfaces moved from `client::ClientStreamHandle` to `trust_dns_proto::DnsStreamHandle`
 - `Message::sign` has been renamed and change to the more general method `Message::finalize`
+- Some `io::Error`s have been converted to `trust_dns_proto::ProtoError`
 
 ### Fixed
 

Cargo.lock

@@ -1298,6 +1298,7 @@ dependencies = [
 name = "trust-dns-resolver"
 version = "0.6.0"
 dependencies = [
+ "error-chain 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)",
  "futures 0.1.16 (registry+https://github.com/rust-lang/crates.io-index)",
  "ipconfig 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "lalrpop 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)",

client/src/client/client.rs

@@ -48,7 +48,6 @@ pub trait Client<C: ClientHandle> {
     /// Get a mutable handle reference tot the Core assiated to the Client
     fn get_client_handle(&self) -> RefMut<C>;
 
-    // FIXME: changed error type
     /// A *classic* DNS query, i.e. does not perform and DNSSec operations
     ///
     /// *Note* As of now, this will not recurse on PTR or CNAME record responses, that is up to

client/src/client/client_connection.rs

@@ -26,7 +26,6 @@ pub trait ClientConnection: Sized {
     /// The associated DNS Message stream type.
     type MessageStream;
 
-    // FIXME: ClientSteamHandle -> DnsStreamHandle
     /// Return the inner Futures items
     ///
     /// Consumes the connection and allows for future based operations afterward.

client/src/client/client_future.rs

@@ -11,36 +11,17 @@ use std::time::Duration;
 
 use futures::Future;
 use futures::stream::Stream;
-use futures::sync::mpsc::UnboundedSender;
 use rand;
 use tokio_core::reactor::Handle;
 use trust_dns_proto::{BasicDnsHandle, DnsStreamHandle, DnsHandle, DnsFuture};
 
+use client::ClientStreamHandle;
 use error::*;
 use op::{Message, MessageType, OpCode, Query, UpdateMessage};
 use rr::{domain, DNSClass, IntoRecordSet, RData, Record, RecordType};
 use rr::dnssec::Signer;
 use rr::rdata::NULL;
 
-/// A reference to a Sender of bytes returned from the creation of a UdpClientStream or TcpClientStream
-pub type StreamHandle = UnboundedSender<Vec<u8>>;
-
-/// Implementations of Sinks for sending DNS messages
-#[deprecated(note = "use [`trust_dns_proto::DnsStreamHandle`] instead")]
-pub trait ClientStreamHandle {
-    /// Sends a message to the Handle for delivery to the server.
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()>;
-}
-
-#[allow(deprecated)]
-impl ClientStreamHandle for StreamHandle {
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()> {
-        UnboundedSender::unbounded_send(self, buffer).map_err(|_| {
-            io::Error::new(io::ErrorKind::Other, "unknown")
-        })
-    }
-}
-
 /// A DNS Client implemented over futures-rs.
 ///
 /// This Client is generic and capable of wrapping UDP, TCP, and other underlying DNS protocol
@@ -51,7 +32,6 @@ pub struct ClientFuture<S: Stream<Item = Vec<u8>, Error = io::Error>> {
 }
 
 impl<S: Stream<Item = Vec<u8>, Error = io::Error> + 'static> ClientFuture<S> {
-    // FIXME: stream_handle type change
     /// Spawns a new ClientFuture Stream. This uses a default timeout of 5 seconds for all requests.
     ///
     /// # Arguments
@@ -64,7 +44,7 @@ impl<S: Stream<Item = Vec<u8>, Error = io::Error> + 'static> ClientFuture<S> {
     /// * `signer` - An optional signer for requests, needed for Updates with Sig0, otherwise not needed
     pub fn new(
         stream: Box<Future<Item = S, Error = io::Error>>,
-        stream_handle: Box<DnsStreamHandle>,
+        stream_handle: Box<ClientStreamHandle>,
         loop_handle: &Handle,
         signer: Option<Signer>,
     ) -> BasicClientHandle {
@@ -77,7 +57,6 @@ impl<S: Stream<Item = Vec<u8>, Error = io::Error> + 'static> ClientFuture<S> {
         )
     }
 
-    // FIXME: stream_handle type change
     /// Spawns a new ClientFuture Stream.
     ///
     /// # Arguments
@@ -141,7 +120,6 @@ pub trait ClientHandle: Clone + DnsHandle<Error = ClientError> {
         false
     }
 
-    // FIXME: changed return type
     /// A *classic* DNS query
     ///
     /// This is identical to `query`, but instead takes a `Query` object.
@@ -173,7 +151,6 @@ pub trait ClientHandle: Clone + DnsHandle<Error = ClientError> {
         self.send(message)
     }
 
-    // FIXME: changed return type
     /// A *classic* DNS query
     ///
     /// *Note* As of now, this will not recurse on PTR or CNAME record responses, that is up to

client/src/client/mod.rs

@@ -31,9 +31,15 @@ pub use self::client::{Client, SyncClient};
 pub use self::client::SecureSyncClient;
 pub use self::client_connection::ClientConnection;
 #[allow(deprecated)]
-pub use self::client_future::{ClientFuture, BasicClientHandle, ClientHandle, StreamHandle,
-                              ClientStreamHandle};
+pub use self::client_future::{ClientFuture, BasicClientHandle, ClientHandle};
 pub use self::memoize_client_handle::MemoizeClientHandle;
 pub use self::retry_client_handle::RetryClientHandle;
 #[cfg(any(feature = "openssl", feature = "ring"))]
 pub use self::secure_client_handle::SecureClientHandle;
+
+/// This is an alias for [`trust_dns_proto::StreamHandle`]
+#[deprecated(note = "use [`trust_dns_proto::StreamHandle`] instead")]
+pub use trust_dns_proto::StreamHandle;
+/// This is an alias for [`trust_dns_proto::DnsStreamHandle`]
+#[deprecated(note = "use [`trust_dns_proto::DnsStreamHandle`] instead")]
+pub use trust_dns_proto::DnsStreamHandle as ClientStreamHandle;

client/src/client/retry_client_handle.rs

@@ -47,7 +47,6 @@ where
 {
     type Error = ClientError;
 
-    // FIXME: this is a type change, generify DnsHandle Result...
     fn send(&mut self, message: Message) -> Box<Future<Item = Message, Error = Self::Error>> {
         // need to clone here so that the retry can resend if necessary...
         //  obviously it would be nice to be lazy about this...

client/src/client/secure_client_handle.rs

@@ -5,7 +5,7 @@
 // http://opensource.org/licenses/MIT>, at your option. This file may not be
 // copied, modified, or distributed except according to those terms.
 
-// FIXME: move to proto
+// TODO: move to proto
 
 use std::clone::Clone;
 use std::collections::HashSet;
@@ -100,7 +100,6 @@ where
 {
     type Error = ClientError;
 
-    // FIXME: this is a type change, generify DnsHandle Result...
     fn send(&mut self, mut message: Message) -> Box<Future<Item = Message, Error = Self::Error>> {
         // backstop, this might need to be configurable at some point
         if self.request_depth > 20 {

client/src/lib.rs

@@ -240,59 +240,20 @@ pub mod tcp;
 pub mod udp;
 pub mod serialize;
 
-use std::io;
 use std::net::SocketAddr;
 
 use futures::sync::mpsc::UnboundedSender;
-use futures::Stream;
 
 use op::Message;
-#[allow(deprecated)]
-use client::ClientStreamHandle;
-
-/// A stream of serialized DNS Messages
-pub type BufStream = Stream<Item = (Vec<u8>, SocketAddr), Error = io::Error>;
 
 /// A sender to which serialized DNS Messages can be sent
 pub type BufStreamHandle = UnboundedSender<(Vec<u8>, SocketAddr)>;
 
-/// A stream of messsages
-pub type MessageStream = Stream<Item = Message, Error = io::Error>;
-
 /// A sender to which a Message can be sent
 pub type MessageStreamHandle = UnboundedSender<Message>;
 
-/// A buffering stream bound to a `SocketAddr`
-pub struct BufClientStreamHandle {
-    name_server: SocketAddr,
-    sender: BufStreamHandle,
-}
-
-impl BufClientStreamHandle {
-    /// Constructs a new Buffered Stream Handle, used for sending data to the DNS peer.
-    ///
-    /// # Arguments
-    ///
-    /// * `name_server` - the address of the DNS server
-    /// * `sender` - the handle being used to send data to the server
-    pub fn new(name_server: SocketAddr, sender: BufStreamHandle) -> Self {
-        BufClientStreamHandle {
-            name_server: name_server,
-            sender: sender,
-        }
-    }
-}
-
-#[allow(deprecated)]
-impl ClientStreamHandle for BufClientStreamHandle {
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()> {
-        let name_server: SocketAddr = self.name_server;
-        let sender: &mut _ = &mut self.sender;
-        sender.unbounded_send((buffer, name_server)).map_err(|_| {
-            io::Error::new(io::ErrorKind::Other, "unknown")
-        })
-    }
-}
+#[deprecated(note = "use [`trust_dns_proto::BufDnsStreamHandle`] instead")]
+pub use trust_dns_proto::BufDnsStreamHandle as BufClientStreamHandle;
 
 /// Returns a version as specified in Cargo.toml
 pub fn version() -> &'static str {

client/src/rr/dnssec/signer.rs

@@ -372,7 +372,6 @@ impl Signer {
     ///
     /// The signature, ready to be stored in an `RData::RRSIG`.
     pub fn sign(&self, tbs: &TBS) -> ProtoResult<Vec<u8>> {
-        // FIXME: this error conversion shouldn't be necessary
         self.key.sign(self.algorithm, tbs).map_err(|e| {
             ProtoErrorKind::Msg(format!("signing error: {}", e)).into()
         })

client/src/tcp/tcp_client_connection.rs

@@ -24,7 +24,7 @@ use tokio_core::reactor::Core;
 use trust_dns_proto::DnsStreamHandle;
 
 use error::*;
-use client::ClientConnection;
+use client::{ClientConnection, ClientStreamHandle};
 use tcp::TcpClientStream;
 
 /// Tcp client connection
@@ -33,7 +33,7 @@ use tcp::TcpClientStream;
 pub struct TcpClientConnection {
     io_loop: Core,
     tcp_client_stream: Box<Future<Item = TcpClientStream<TcpStream>, Error = io::Error>>,
-    client_stream_handle: Box<DnsStreamHandle>,
+    client_stream_handle: Box<ClientStreamHandle>,
 }
 
 impl TcpClientConnection {

client/src/udp/udp_client_connection.rs

@@ -22,7 +22,7 @@ use tokio_core::reactor::Core;
 use trust_dns_proto::DnsStreamHandle;
 
 use error::*;
-use client::ClientConnection;
+use client::{ClientConnection, ClientStreamHandle};
 use udp::UdpClientStream;
 
 /// UDP based DNS Client connection
@@ -31,7 +31,7 @@ use udp::UdpClientStream;
 pub struct UdpClientConnection {
     io_loop: Core,
     udp_client_stream: Box<Future<Item = UdpClientStream, Error = io::Error>>,
-    client_stream_handle: Box<DnsStreamHandle>,
+    client_stream_handle: Box<ClientStreamHandle>,
 }
 
 impl UdpClientConnection {

proto/README.md

@@ -2,6 +2,8 @@
 
 TRust-DNS Proto is the foundational DNS protocol library and implementation for TRust-DNS. It is not expected to be used directly. Please see TRust-DNS [Resolver](https://crates.io/crates/trust-dns-resolver), [Client](https://crates.io/crates/trust-dns), or [Server](https://crates.io/crates/trust-dns-server) for higher level interfaces.
 
+*WARNING* The Proto crate is designed as an internal layer in the TRust-DNS ecosystem, it will change potentially in breaking ways, and should not generally be used directly. Please see the Resolver, Client or Server for more stable interfaces.
+
 ## Versioning
 
 TRust-DNS does it's best job to follow semver. TRust-DNS will be promoted to 1.0 upon stabilization of the publicly exposed APIs. This does not mean that TRust-DNS will necessarily break on upgrades between 0.x updates. Whenever possible, old APIs will be deprecated with notes on what replaced those deprecations. TRust-DNS will make a best effort to never break software which depends on it due to API changes, though this can not be guaranteed. Deprecated interfaces will be maintained for at minimum one major release after that in which they were deprecated (where possible), with the exception of the upgrade to 1.0 where all deprecated interfaces will be planned to be removed.

proto/src/dns_handle.rs

@@ -24,16 +24,19 @@ use op::{Message, MessageFinalizer, OpCode};
 
 const QOS_MAX_RECEIVE_MSGS: usize = 100; // max number of messages to receive from the UDP socket
 
+/// The StreamHandle is the general interface for communicating with the DnsFuture
+pub type StreamHandle = UnboundedSender<Vec<u8>>;
+
 /// Implementations of Sinks for sending DNS messages
 pub trait DnsStreamHandle {
     /// Sends a message to the Handle for delivery to the server.
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()>;
+    fn send(&mut self, buffer: Vec<u8>) -> ProtoResult<()>;
 }
 
-impl DnsStreamHandle for UnboundedSender<Vec<u8>> {
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()> {
-        UnboundedSender::unbounded_send(self, buffer).map_err(|_| {
-            io::Error::new(io::ErrorKind::Other, "unknown")
+impl DnsStreamHandle for StreamHandle {
+    fn send(&mut self, buffer: Vec<u8>) -> ProtoResult<()> {
+        UnboundedSender::unbounded_send(self, buffer).map_err(|e| {
+            ProtoErrorKind::Msg(format!("mpsc::SendError {}", e)).into()
         })
     }
 }

proto/src/lib.rs

@@ -31,10 +31,8 @@ extern crate tokio_io;
 #[cfg(feature = "ring")]
 extern crate untrusted;
 
-use std::io;
 use std::net::SocketAddr;
 
-use futures::Stream;
 use futures::sync::mpsc::UnboundedSender;
 
 mod dns_handle;
@@ -45,21 +43,14 @@ pub mod serialize;
 pub mod tcp;
 pub mod udp;
 
-pub use dns_handle::{BasicDnsHandle, DnsFuture, DnsHandle, DnsStreamHandle};
+pub use dns_handle::{BasicDnsHandle, DnsFuture, DnsHandle, DnsStreamHandle, StreamHandle};
 use op::Message;
-
-
-// FIXME: change io::Error to error::ProtoError
-/// A stream of serialized DNS Messages
-pub type BufStream = Stream<Item = (Vec<u8>, SocketAddr), Error = io::Error>;
+use error::*;
 
 // TODO: change to Sink
 /// A sender to which serialized DNS Messages can be sent
 pub type BufStreamHandle = UnboundedSender<(Vec<u8>, SocketAddr)>;
 
-/// A stream of messsages
-pub type MessageStream = Stream<Item = Message, Error = io::Error>;
-
 // TODO: change to Sink
 /// A sender to which a Message can be sent
 pub type MessageStreamHandle = UnboundedSender<Message>;
@@ -86,11 +77,11 @@ impl BufDnsStreamHandle {
 }
 
 impl DnsStreamHandle for BufDnsStreamHandle {
-    fn send(&mut self, buffer: Vec<u8>) -> io::Result<()> {
+    fn send(&mut self, buffer: Vec<u8>) -> ProtoResult<()> {
         let name_server: SocketAddr = self.name_server;
         let sender: &mut _ = &mut self.sender;
-        sender.unbounded_send((buffer, name_server)).map_err(|_| {
-            io::Error::new(io::ErrorKind::Other, "unknown")
+        sender.unbounded_send((buffer, name_server)).map_err(|e| {
+            ProtoErrorKind::Msg(format!("mpsc::SendError {}", e)).into()
         })
     }
 }

proto/src/serialize/binary/bin_tests.rs

@@ -13,7 +13,6 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-// FIXME: move this to proto/tests
 
 use std::fmt::Debug;
 use error::*;

proto/src/serialize/mod.rs

@@ -16,5 +16,4 @@
 
 //! Contains serialization libraries for `binary` and text, `txt`.
 
-// FIXME: rename and relocate
 pub mod binary;

resolver/Cargo.toml

@@ -50,6 +50,7 @@ path = "src/lib.rs"
 lalrpop = "^0.13.1"
 
 [dependencies]
+error-chain = "0.1.12"
 futures = "^0.1.6"
 lalrpop-util = "^0.13.1"
 log = "^0.3.5"

resolver/src/error.rs

@@ -0,0 +1,36 @@
+#![allow(missing_docs)]
+
+
+error_chain! {
+    // The type defined for this error. These are the conventional
+    // and recommended names, but they can be arbitrarily chosen.
+    types {
+        Error, ErrorKind, ChainErr, Result;
+    }
+
+    // Automatic conversions between this error chain and other
+    // error chains. In this case, it will e.g. generate an
+    // `ErrorKind` variant called `Dist` which in turn contains
+    // the `rustup_dist::ErrorKind`, with conversions from
+    // `rustup_dist::Error`.
+    //
+    // This section can be empty.
+    links {
+        ::trust_dns_proto::error::ProtoError, ::trust_dns_proto::error::ProtoErrorKind, ProtoError;
+    }
+
+    // Automatic conversions between this error chain and other
+    // error types not defined by the `error_chain!`. These will be
+    // boxed as the error cause and wrapped in a new error with,
+    // in this case, the `ErrorKind::Temp` variant.
+    //
+    // This section can be empty.
+    foreign_links {
+      ::std::io::Error, Io, "io error";
+    }
+
+    // Define additional `ErrorKind` variants. The syntax here is
+    // the same as `quick_error!`, but the `from()` and `cause()`
+    // syntax is not supported.
+    errors {}
+}

resolver/src/lib.rs

@@ -125,6 +125,8 @@
 
 #![deny(missing_docs)]
 
+#[macro_use]
+extern crate error_chain;
 extern crate futures;
 extern crate lalrpop_util;
 #[macro_use]
@@ -137,6 +139,7 @@ extern crate trust_dns_proto;
 extern crate ipconfig;
 
 pub mod config;
+pub mod error;
 pub mod lookup_ip;
 pub mod lookup;
 pub mod lookup_state;

resolver/src/lookup.rs

@@ -8,7 +8,7 @@
 
 //! Lookup result from a resolution of ipv4 and ipv6 records with a Resolver.
 
-use std::error::Error;
+use std::error::Error as StdError;
 use std::io;
 use std::net::{Ipv4Addr, Ipv6Addr};
 use std::mem;
@@ -84,10 +84,10 @@ pub enum LookupEither<C: ClientHandle + 'static, P: ConnectionProvider<ConnHandl
 }
 
 impl<C: ClientHandle, P: ConnectionProvider<ConnHandle = C>> DnsHandle for LookupEither<C, P> {
-    // FIXME: make this a ResolverError
+    // TODO: this should be a ResolverError.
     type Error = ClientError;
 
-    fn send(&mut self, message: Message) -> Box<Future<Item = Message, Error = ClientError>> {
+    fn send(&mut self, message: Message) -> Box<Future<Item = Message, Error = Self::Error>> {
         match *self {
             LookupEither::Retry(ref mut c) => c.send(message),
             LookupEither::Secure(ref mut c) => c.send(message),
@@ -162,7 +162,7 @@ impl<C: ClientHandle + 'static> InnerLookupFuture<C> {
         }
     }
 
-    pub(crate) fn error<E: Error>(client_cache: CachingClient<C>, error: E) -> Self {
+    pub(crate) fn error<E: StdError>(client_cache: CachingClient<C>, error: E) -> Self {
         return InnerLookupFuture {
             // errors on names don't need to be cheap... i.e. this clone is unfortunate in this case.
             client_cache,

resolver/src/lookup_ip.rs

@@ -257,7 +257,6 @@ fn rt_then_swap<C: ClientHandle + 'static>(
             .then(move |res| {
                 match res {
                     Ok(ips) => {
-                        // FIXME: lookup should return an Option, Option::None should be cached for NxDomain
                         if ips.is_empty() {
                             // no ips returns, NXDomain or Otherwise, doesn't matter
                             Box::new(

resolver/src/lookup_state.rs

@@ -152,7 +152,7 @@ impl DnsLru {
 #[derive(Clone, Debug)]
 #[doc(hidden)]
 pub struct CachingClient<C: ClientHandle> {
-    // FIXME: switch to FuturesMutex (Mutex will have some undesireable locking)
+    // TODO: switch to FuturesMutex (Mutex will have some undesireable locking)
     lru: Arc<Mutex<DnsLru>>,
     client: C,
 }

resolver/src/name_server_pool.rs

@@ -405,7 +405,7 @@ impl<C: ClientHandle, P: ConnectionProvider<ConnHandle = C>> Eq for NameServer<C
 /// This is not expected to be used directly, see `ResolverFuture`.
 #[derive(Clone)]
 pub struct NameServerPool<C: ClientHandle + 'static, P: ConnectionProvider<ConnHandle = C> + 'static> {
-    // FIXME: switch to FuturesMutex (Mutex will have some undesireable locking)
+    // TODO: switch to FuturesMutex (Mutex will have some undesireable locking)
     datagram_conns: Arc<Mutex<BinaryHeap<NameServer<C, P>>>>, /* All NameServers must be the same type */
     stream_conns: Arc<Mutex<BinaryHeap<NameServer<C, P>>>>, /* All NameServers must be the same type */
     options: ResolverOpts,