Bfry/no panic (#42)

* mark all valid panic points

* OpCode no longer panic!()

* Changelog message for #37

CHANGELOG.md

@@ -5,6 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## unreleased
 ### Fixed
 - Randomized ports for client connections and message ids, #23
+- OpCode::From for u8 removed, added OpCode::from_u8(), #36 
 
 ### Changed
 - Cleaned up the Server implementation to isolate connection handlers

src/authority/authority.rs

@@ -232,7 +232,7 @@ impl Authority {
     if let &RData::SOA(ref soa_rdata) = soa.get_rdata() {
       soa_rdata.get_serial()
     } else {
-      panic!("This was not an SOA record");
+      panic!("This was not an SOA record"); // valid panic, never should happen
     }
   }
 
@@ -248,7 +248,7 @@ impl Authority {
       soa_rdata.increment_serial();
       soa_rdata.get_serial()
     } else {
-      panic!("This was not an SOA record");
+      panic!("This was not an SOA record"); // valid panic, never should happen
     };
 
     self.upsert(soa, serial);
@@ -1158,7 +1158,7 @@ pub mod authority_tests {
       assert_eq!(result.first().unwrap().get_dns_class(), DNSClass::IN);
       assert_eq!(result.first().unwrap().get_rdata(), &RData::A(Ipv4Addr::new(93,184,216,34)));
     } else {
-      panic!("expected a result");
+      panic!("expected a result");  // valid panic, in test
     }
   }
 
@@ -1177,7 +1177,7 @@ pub mod authority_tests {
       assert_eq!(result.first().unwrap().get_dns_class(), DNSClass::IN);
       assert_eq!(result.first().unwrap().get_rdata(), &RData::A(Ipv4Addr::new(93,184,216,34)));
     } else {
-      panic!("expected a result");
+      panic!("expected a result");  // valid panic, in test
     }
   }
 

src/authority/persistence.rs

@@ -178,7 +178,7 @@ impl Journal {
       match self.version + 1 {
         0 => self.version = try!(self.init_up()),
         1 => self.version = try!(self.records_up()),
-        _ => panic!("incorrect version somewhere"),
+        _ => panic!("incorrect version somewhere"), // valid panic, non-recoverable state
       }
 
       try!(self.update_schema_version(self.version));

src/authority/rr_set.rs

@@ -167,7 +167,7 @@ impl RRSet {
                 return false;
               }
             },
-            rdata @ _ => panic!("wrong rdata: {:?}", rdata),
+            rdata @ _ => panic!("wrong rdata: {:?}", rdata), // valid panic, never should happen
           }
         }
 

src/client/client.rs

@@ -215,11 +215,11 @@ impl<C: ClientConnection> Client<C> {
                      if let &RData::SIG(ref sig) = rrsig.get_rdata() { sig.get_type_covered() } else { RecordType::NULL });
             }
           } else {
-            panic!("this should be a DNSKEY")
+            panic!("this should be a DNSKEY") // valid panic, never should happen
           }
         }
       } else {
-        panic!("expected RRSIG: {:?}", rrsig.get_rr_type());
+        panic!("expected RRSIG: {:?}", rrsig.get_rr_type()); // valid panic, never should happen
       }
     }
 
@@ -277,7 +277,7 @@ impl<C: ClientConnection> Client<C> {
           return Ok(proof)
         }
       } else {
-        panic!("expected DS: {:?}", ds.get_rr_type());
+        panic!("expected DS: {:?}", ds.get_rr_type()); // valid panic, never should happen
       }
     }
 
@@ -334,8 +334,6 @@ impl<C: ClientConnection> Client<C> {
   //  NSEC and RRSIG bits in an NSEC RR.
   fn verify_nsec(&self, query_name: &domain::Name, query_type: RecordType,
                  _: DNSClass, nsecs: Vec<&Record>) -> ClientResult<()> {
-    debug!("verifying nsec");
-
     // first look for a record with the same name
     //  if they are, then the query_type should not exist in the NSEC record.
     //  if we got an NSEC record of the same name, but it is listed in the NSEC types,
@@ -344,7 +342,7 @@ impl<C: ClientConnection> Client<C> {
       if let &RData::NSEC(ref rdata) = r.get_rdata() {
         !rdata.get_type_bit_maps().contains(&query_type)
       } else {
-        panic!("expected NSEC was {:?}", r.get_rr_type())
+        panic!("expected NSEC was {:?}", r.get_rr_type()) // valid panic, never should happen
       }
     }) { return Ok(()) }
 
@@ -353,7 +351,7 @@ impl<C: ClientConnection> Client<C> {
       if let &RData::NSEC(ref rdata) = r.get_rdata() {
         query_name < rdata.get_next_domain_name()
       } else {
-        panic!("expected NSEC was {:?}", r.get_rr_type())
+        panic!("expected NSEC was {:?}", r.get_rr_type()) // valid panic, never should happen
       }
     }) { return Ok(()) }
 

src/op/edns.rs

@@ -75,7 +75,7 @@ impl<'a> From<&'a Record> for Edns {
       },
       _ => {
         // this should be a coding error, as opposed to a parsing error.
-        panic!("rr_type doesn't match the RData: {:?}", value.get_rdata());
+        panic!("rr_type doesn't match the RData: {:?}", value.get_rdata()); // valid panic, never should happen
       },
     };
 

src/op/header.rs

@@ -270,7 +270,7 @@ impl BinSerializable<Header> for Header {
     // if the first bit is set
     let message_type = if (0x80 & q_opcd_a_t_r) == 0x80 { MessageType::Response } else { MessageType::Query };
     // the 4bit opcode, masked and then shifted right 3bits for the u8...
-    let op_code: OpCode = ((0x78 & q_opcd_a_t_r) >> 3).into();
+    let op_code: OpCode = try!(OpCode::from_u8((0x78 & q_opcd_a_t_r) >> 3));
     let authoritative = (0x4 & q_opcd_a_t_r) == 0x4;
     let truncation = (0x2 & q_opcd_a_t_r) == 0x2;
     let recursion_desired = (0x1 & q_opcd_a_t_r) == 0x1;

src/op/op_code.rs

@@ -18,6 +18,8 @@
 
 use std::convert::From;
 
+use ::error::*;
+
 /// Operation code for queries, updates, and responses
 ///
 /// [RFC 1035, DOMAIN NAMES - IMPLEMENTATION AND SPECIFICATION, November 1987](https://tools.ietf.org/html/rfc1035)
@@ -57,11 +59,11 @@ pub enum OpCode {
 /// use std::convert::From;
 /// use trust_dns::op::op_code::OpCode;
 ///
-/// let var: OpCode = From::from(0);
-/// assert_eq!(OpCode::Query, var);
+/// let var: u8 = From::from(OpCode::Query);
+/// assert_eq!(0, var);
 ///
-/// let var: OpCode = 0.into();
-/// assert_eq!(OpCode::Query, var);
+/// let var: u8 = OpCode::Query.into();
+/// assert_eq!(0, var);
 /// ```
 impl From<OpCode> for u8 {
   fn from(rt: OpCode) -> Self {
@@ -83,20 +85,17 @@ impl From<OpCode> for u8 {
 /// use std::convert::From;
 /// use trust_dns::op::op_code::OpCode;
 ///
-/// let var: u8 = From::from(OpCode::Query);
-/// assert_eq!(0, var);
-///
-/// let var: u8 = OpCode::Query.into();
-/// assert_eq!(0, var);
+/// let var: OpCode = OpCode::from_u8(0).unwrap();
+/// assert_eq!(OpCode::Query, var);
 /// ```
-impl From<u8> for OpCode {
-  fn from(value: u8) -> Self {
+impl OpCode {
+  pub fn from_u8(value: u8) -> DecodeResult<Self> {
     match value {
-      0 => OpCode::Query,
-      2 => OpCode::Status,
-      4 => OpCode::Notify,
-      5 => OpCode::Update,
-      _ => panic!("unimplemented code: {}", value),
+      0 => Ok(OpCode::Query),
+      2 => Ok(OpCode::Status),
+      4 => Ok(OpCode::Notify),
+      5 => Ok(OpCode::Update),
+      _ => Err(DecodeErrorKind::Msg(format!("unknown OpCode: {}", value)).into()),
     }
   }
 }

src/rr/dnssec/algorithm.rs

@@ -13,6 +13,8 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+use std::str::FromStr;
+
 use openssl::crypto::pkey::{PKey, Role};
 use openssl::crypto::rsa::RSA;
 use openssl::bn::BigNum;
@@ -108,7 +110,7 @@ pub enum Algorithm {
 
 impl Algorithm {
   pub fn sign(&self, private_key: &PKey, data: &[u8]) -> Vec<u8> {
-    if !private_key.can(Role::Sign) { panic!("This key cannot be used for signing") }
+    assert!(private_key.can(Role::Sign), "This key cannot be used for signing");
 
     // calculate the hash...
     let hash = DigestType::from(*self).hash(data);
@@ -118,7 +120,7 @@ impl Algorithm {
   }
 
   pub fn verify(&self, public_key: &PKey, data: &[u8], signature: &[u8]) -> bool {
-    if !public_key.can(Role::Verify) { panic!("This key cannot be used to verify signature") }
+    assert!(public_key.can(Role::Verify), "This key cannot be used to verify signature");
 
     // calculate the hash on the local data
     let hash = DigestType::from(*self).hash(data);
@@ -247,16 +249,18 @@ impl BinSerializable<Algorithm> for Algorithm {
   }
 }
 
-impl From<&'static str> for Algorithm {
-  fn from(s: &'static str) -> Algorithm {
+impl FromStr for Algorithm {
+  type Err = DecodeError;
+
+  fn from_str(s: &str) -> DecodeResult<Algorithm> {
     match s {
-      "RSASHA1" => Algorithm::RSASHA1,
-      "RSASHA256" => Algorithm::RSASHA256,
-      "RSASHA1-NSEC3-SHA1" => Algorithm::RSASHA1NSEC3SHA1,
-      "RSASHA512" => Algorithm::RSASHA512,
+      "RSASHA1" => Ok(Algorithm::RSASHA1),
+      "RSASHA256" => Ok(Algorithm::RSASHA256),
+      "RSASHA1-NSEC3-SHA1" => Ok(Algorithm::RSASHA1NSEC3SHA1),
+      "RSASHA512" => Ok(Algorithm::RSASHA512),
 //      "ECDSAP256SHA256" => Algorithm::ECDSAP256SHA256,
 //      "ECDSAP384SHA384" => Algorithm::ECDSAP384SHA384,
-      _ => panic!("unrecognized string {}", s),
+      _ => Err(DecodeErrorKind::Msg(format!("unrecognized string {}", s)).into()),
     }
   }
 }

src/rr/dnssec/digest_type.rs

@@ -78,7 +78,7 @@ impl From<DigestType> for u8 {
       DigestType::SHA256 => 2,
       // DigestType::GOSTR34_11_94 => 3,
       DigestType::SHA384 => 4,
-      _ => panic!("No code for this type: {:?}", a)
+      DigestType::SHA512 => 255,
     }
   }
 }

src/rr/record_data.rs

@@ -643,23 +643,23 @@ impl RData {
     let rdata = match record_type {
       RecordType::A => RData::A(try!(rdata::a::parse(tokens))),
       RecordType::AAAA => RData::AAAA(try!(rdata::aaaa::parse(tokens))),
-      RecordType::ANY => panic!("parsing ANY doesn't make sense"),
-      RecordType::AXFR => panic!("parsing AXFR doesn't make sense"),
+      RecordType::ANY => panic!("parsing ANY doesn't make sense"), // valid panic, never should happen
+      RecordType::AXFR => panic!("parsing AXFR doesn't make sense"), // valid panic, never should happen
       RecordType::CNAME => RData::CNAME(try!(rdata::name::parse(tokens, origin))),
-      RecordType::KEY => panic!("KEY should be dynamically generated"),
-      RecordType::DNSKEY => panic!("DNSKEY should be dynamically generated"),
-      RecordType::DS => panic!("DS should be dynamically generated"),
-      RecordType::IXFR => panic!("parsing IXFR doesn't make sense"),
+      RecordType::KEY => panic!("KEY should be dynamically generated"), // valid panic, never should happen
+      RecordType::DNSKEY => panic!("DNSKEY should be dynamically generated"), // valid panic, never should happen
+      RecordType::DS => panic!("DS should be dynamically generated"), // valid panic, never should happen
+      RecordType::IXFR => panic!("parsing IXFR doesn't make sense"), // valid panic, never should happen
       RecordType::MX => RData::MX(try!(rdata::mx::parse(tokens, origin))),
       RecordType::NULL =>  RData::NULL(try!(rdata::null::parse(tokens))),
       RecordType::NS => RData::NS(try!(rdata::name::parse(tokens, origin))),
-      RecordType::NSEC => panic!("NSEC should be dynamically generated"),
-      RecordType::NSEC3 => panic!("NSEC3 should be dynamically generated"),
-      RecordType::NSEC3PARAM => panic!("NSEC3PARAM should be dynamically generated"),
-      RecordType::OPT => panic!("parsing OPT doesn't make sense"),
+      RecordType::NSEC => panic!("NSEC should be dynamically generated"), // valid panic, never should happen
+      RecordType::NSEC3 => panic!("NSEC3 should be dynamically generated"), // valid panic, never should happen
+      RecordType::NSEC3PARAM => panic!("NSEC3PARAM should be dynamically generated"), // valid panic, never should happen
+      RecordType::OPT => panic!("parsing OPT doesn't make sense"), // valid panic, never should happen
       RecordType::PTR => RData::PTR(try!(rdata::name::parse(tokens, origin))),
-      RecordType::RRSIG => panic!("RRSIG should be dynamically generated"),
-      RecordType::SIG => panic!("parsing SIG doesn't make sense"),
+      RecordType::RRSIG => panic!("RRSIG should be dynamically generated"), // valid panic, never should happen
+      RecordType::SIG => panic!("parsing SIG doesn't make sense"), // valid panic, never should happen
       RecordType::SOA => RData::SOA(try!(rdata::soa::parse(tokens, origin))),
       RecordType::SRV => RData::SRV(try!(rdata::srv::parse(tokens, origin))),
       RecordType::TXT => RData::TXT(try!(rdata::txt::parse(tokens))),

src/serialize/txt/master_lex.rs

@@ -229,7 +229,7 @@ mod lex_test {
 
   fn next_token(lexer: &mut Lexer) -> Option<Token> {
     let result = lexer.next_token();
-    if result.is_err() { panic!("{:?}", result) }
+    assert!(!result.is_err(), "{:?}", result);
     result.unwrap()
   }
 

src/serialize/txt/txt_tests.rs

@@ -61,7 +61,7 @@ venera  A       10.1.0.52
     assert_eq!(3600000, soa.get_expire());
     assert_eq!(60, soa.get_minimum());
   } else {
-    panic!("Not an SOA record!!!")
+    panic!("Not an SOA record!!!") // valid panic, test code
   }
 
   // NS
@@ -84,7 +84,7 @@ venera  A       10.1.0.52
     if let RData::NS( ref nsdname ) = *record.get_rdata() {
       assert_eq!(name, nsdname);
     } else {
-      panic!("Not an NS record!!!")
+      panic!("Not an NS record!!!") // valid panic, test code
     }
   }
 
@@ -109,7 +109,7 @@ venera  A       10.1.0.52
       assert_eq!(num, rdata.get_preference());
       assert_eq!(name, rdata.get_exchange());
     } else {
-      panic!("Not an NS record!!!")
+      panic!("Not an NS record!!!") // valid panic, test code
     }
   }
 
@@ -122,7 +122,7 @@ venera  A       10.1.0.52
   if let RData::A(ref address) = *a_record.get_rdata() {
     assert_eq!(&Ipv4Addr::new(26u8,3u8,0u8,103u8), address);
   } else {
-    panic!("Not an A record!!!")
+    panic!("Not an A record!!!") // valid panic, test code
   }
 
   // AAAA
@@ -131,7 +131,7 @@ venera  A       10.1.0.52
   if let RData::AAAA(ref address) = *aaaa_record.get_rdata() {
     assert_eq!(&Ipv6Addr::from_str("4321:0:1:2:3:4:567:89ab").unwrap(), address);
   } else {
-    panic!("Not a AAAA record!!!")
+    panic!("Not a AAAA record!!!") // valid panic, test code
   }
 
   // SHORT
@@ -141,7 +141,7 @@ venera  A       10.1.0.52
   if let RData::A(ref address) = *short_record.get_rdata() {
     assert_eq!(&Ipv4Addr::new(26u8,3u8,0u8,104u8), address);
   } else {
-    panic!("Not an A record!!!")
+    panic!("Not an A record!!!") // valid panic, test code
   }
 
   // TXT
@@ -165,7 +165,7 @@ venera  A       10.1.0.52
     if let RData::TXT(ref rdata) = *record.get_rdata() {
       assert_eq!(vector as &[String], rdata.get_txt_data());
     } else {
-      panic!("Not a TXT record!!!")
+      panic!("Not a TXT record!!!") // valid panic, test code
     }
   }
 
@@ -174,7 +174,7 @@ venera  A       10.1.0.52
   if let RData::PTR( ref ptrdname ) = *ptr_record.get_rdata() {
     assert_eq!(&Name::new().label("a").label("isi").label("edu"), ptrdname);
   } else {
-    panic!("Not a PTR record!!!")
+    panic!("Not a PTR record!!!") // valid panic, test code
   }
 
   // SRV
@@ -185,6 +185,6 @@ venera  A       10.1.0.52
     assert_eq!(rdata.get_port(), 3);
     assert_eq!(rdata.get_target(), &Name::new().label("short").label("isi").label("edu"));
   } else {
-    panic!("Not an SRV record!!!")
+    panic!("Not an SRV record!!!") // valid panic, test code
   }
 }

src/udp/handler.rs

@@ -130,7 +130,7 @@ impl UdpHandler {
           Ok(UdpState::Writing)
         }
       },
-      UdpState::Done => panic!("This handler should have been removed or reset"),
+      UdpState::Done => panic!("This handler should have been removed or reset"), // valid panic, never should happen
     }
   }
 }