[1198] Changes as a result of review

src/lib/datasrc/database.cc

@@ -409,9 +409,9 @@ DatabaseClient::Finder::findDelegationPoint(const isc::dns::Name& name,
     //
     // The one case where this is forbidden is when we search past the zone
     // cut but the match we find for the glue is a wildcard match.  In that
-    // case, we return the delegation instead.  To save a new search, we record
+    // case, we return the delegation instead (see RFC 1034, section 4.3.3).
-    // the location of the delegation cut when we encounter it here.
+    // To save a new search, we record the location of the delegation cut when
-    // TODO: where does it say we can't return wildcard glue?
+    // we encounter it here. 
     isc::dns::ConstRRsetPtr first_ns;
 
     // We want to search from the apex down.  We are given the full domain
@@ -536,7 +536,8 @@ DatabaseClient::Finder::findWildcardMatch(
         const string wildcard("*." + superdomain.toText());
         const string construct_name(name.toText());
 
-        // TODO What do we do about DNAME here?
+        // TODO Add a check for DNAME, as DNAME wildcards are discouraged (see
+        // RFC 4592 section 4.4).
         // Search for a match.  The types are the same as with original query.
         FoundRRsets found = getRRsets(wildcard, final_types, true,
                                       &construct_name);
@@ -565,7 +566,6 @@ DatabaseClient::Finder::findWildcardMatch(
                 result_status = DELEGATION;
                 result_rrset = dresult.first_ns;
 
-
             } else if (!hasSubdomains(name.split(i - 1).toText())) {
                 // We found a wildcard match and we are sure that the match
                 // is not an empty non-terminal (E.g. searching for a match
@@ -683,8 +683,7 @@ DatabaseClient::Finder::findNoNameResult(const Name& name, const RRType& type,
     if (hasSubdomains(name.toText())) {
         // Does the domain have a subdomain (i.e. it is an empty non-terminal)?
         // If so, return NXRRSET instead of NXDOMAIN (as although the name does
-        // not exist in the zone, it does exist in the DNS tree).
+        // not exist in the database, it does exist in the DNS tree).
-        // pretend something is here as well.
         LOG_DEBUG(logger, DBG_TRACE_DETAILED,
                   DATASRC_DATABASE_FOUND_EMPTY_NONTERMINAL).
             arg(accessor_->getDBName()).arg(name);
@@ -693,15 +692,14 @@ DatabaseClient::Finder::findNoNameResult(const Name& name, const RRType& type,
 
     } else if ((options & NO_WILDCARD) == 0) {
         // It's not an empty non-terminal and wildcard matching is not
-        // disabled, so check for wildcards.
+        // disabled, so check for wildcards. If there is a wildcard match
+        // (i.e. all results except NXDOMAIN) return it; otherwise fall
+        // through to the NXDOMAIN case below.
         const ZoneFinder::FindResult wresult =
             findWildcardMatch(name, type, options, dresult);
-        if (wresult.code == NXDOMAIN && dnssec_data) {
+        if (wresult.code != NXDOMAIN) {
-            // No match on a wildcard, so return the covering NSEC if DNSSEC
+            return (FindResult(wresult.code, wresult.rrset));
-            // data was requested.
-            return (FindResult(NXDOMAIN, findNSECCover(name)));
         }
-        return (FindResult(wresult.code, wresult.rrset));
     }
 
     // All avenues to find a match are now exhausted, return NXDOMAIN (plus
@@ -759,8 +757,8 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
     // name/type/class.  However, there are special cases:
     // - Requested name has a singleton CNAME record associated with it
     // - Requested name is a delegation point (NS only but not at the zone
-    //   apex - DNAME is ignored here).
+    //   apex - DNAME is ignored here as it redirects DNS names subordinate to
-    // TODO: Why is DNAME ignored?
+    //   the owner name - the owner name itself is not redirected.)
     const bool is_origin = (name == getOrigin());
     WantedTypes final_types(FINAL_TYPES());
     final_types.insert(type);
@@ -784,19 +782,10 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
 
     } else if (type != RRType::CNAME() && cni != found.second.end()) {
         // We are not searching for a CNAME but nevertheless we have found one
-        // at the name we are searching so we return it. (A resolver could have
+        // at the name we are searching so we return it. (The caller may
-        // originated the query that caues this result.  If so, it will restart
+        // want to continue the lookup by replacing the query name with the
-        // the resolution process with the name that is the target of this
+        // canonical name and the original RR type.) First though, do a sanity
-        // CNAME.) First though, do a sanity check to ensure that there is
+        // check to ensure that there is only one RR in the CNAME RRset.
-        // only one RR in the CNAME RRset.
-        //
-        // TODO: Check that throwing an exception here is correct.
-        // Unless the exception is caught higher up (probably not, given the
-        // general nature of the exception), it is probably better to log
-        // and error and terminate the query with SERVFAIL instead of crashing
-        // the server.  Although the CNAME is a singleton and multiple RRs
-        // in the RRset may indicate an error in the data, it does not mean
-        // that the entire database is corrupt.
         if (cni->second->getRdataCount() != 1) {
             isc_throw(DataSourceError, "CNAME with " <<
                       cni->second->getRdataCount() << " rdata at " << name <<

src/lib/datasrc/datasrc_messages.mes

@@ -79,11 +79,10 @@ an error, so we set it to the lowest value we found (but we don't modify the
 database). The data in database should be checked and fixed.
 
 % DATASRC_DATABASE_FOUND_CNAME search in datasource %1 for %2/%3/%4 found CNAME
-When searching the domain for a name a CNAME was found at that name.  Even
+When searching the domain for a name a CNAME was found at that name.
-though it was not the RR type being sought, it is returned.  If the query
+Even though it was not the RR type being sought, it is returned.  (The
-of the database was issued by a result searching for the name, the return of
+caller may want to continue the lookup by replacing the query name with
-the CNAME record will cause that resolver to issue another query for the
+the canonical name and restarting the query with the original RR type.)
-target of the CNAME.
 
 % DATASRC_DATABASE_FOUND_DELEGATION Found delegation at %2 in %1
 When searching for a domain, the program met a delegation to a different zone
@@ -116,7 +115,7 @@ name and class, but not for the given type.
 % DATASRC_DATABASE_FOUND_NXRRSET_NSEC search in datasource %1 for %2/%3/%4 resulted in RRset %5
 A search in the database for RRs for the specified name, type and class has
 located RRs that match the name and class but not the type.  DNSSEC information
-has been requested, but there is no NSEC record corresponding to the node.
+has been requested and returned.
 
 % DATASRC_DATABASE_FOUND_RRSET search in datasource %1 resulted in RRset %5
 The data returned by the database backend contained data for the given domain

src/lib/datasrc/zone.h

@@ -264,12 +264,15 @@ public:
     ///   proof of the non existence of any matching wildcard or non existence
     ///   of an exact match when a wildcard match is found.
     ///
-    /// A derived version of this method may involve internal resource
+    /// \exception std::bad_alloc Memory allocation such as for constructing
-    /// allocation, especially for constructing the resulting RRset, and may
+    ///  the resulting RRset fails
-    /// throw an exception if it fails.
+    /// \exception DataSourceError Derived class specific exception, e.g.
-    /// It throws DuplicateRRset exception if there are duplicate rrsets under
+    /// when encountering a bad zone configuration or database connection
-    /// the same domain.
+    /// failure.  Although these are considered rare, exceptional events,
-    /// It should not throw other types of exceptions.
+    /// it can happen under relatively usual conditions (unlike memory
+    /// allocation failure).  So, in general, the application is expected
+    /// to catch this exception, either specifically or as a result of
+    /// catching a base exception class, and handle it gracefully.
     ///
     /// \param name The domain name to be searched for.
     /// \param type The RR type to be searched for.

tools/reorder_message_file.py

@@ -26,7 +26,7 @@
 
 import sys
 
-def removeEmptyLeadingTrailing(lines):
+def remove_empty_leading_trailing(lines):
     """
     Removes leading and trailing empty lines.
 
@@ -124,7 +124,7 @@ def make_dict(lines):
     while index < len(lines):
         if lines[index].startswith("%"):
             # Start of new message
-            dictionary[message_key] = removeEmptyLeadingTrailing(message_lines)
+            dictionary[message_key] = remove_empty_leading_trailing(message_lines)
             message_key = canonicalise_message_line(lines[index])
             message_lines = [message_key]
         else:
@@ -132,7 +132,7 @@ def make_dict(lines):
 
         index = index + 1
 
-    dictionary[message_key] = removeEmptyLeadingTrailing(message_lines)
+    dictionary[message_key] = remove_empty_leading_trailing(message_lines)
 
     return dictionary
 
@@ -157,7 +157,7 @@ def print_dict(dictionary):
             print(l.strip())
 
 
-def processFile(filename):
+def process_file(filename):
     """
     Processes a file by reading it and searching for the first line starting
     with the '%' sign.  Everything before that line is treated as the file
@@ -193,4 +193,4 @@ if __name__ == "__main__":
     if len(sys.argv) != 2:
         print "Usage: python reorder.py message_file"
     else:
-        processFile(sys.argv[1])
+        process_file(sys.argv[1])