From 52802deb18632b028815d25f19976d0576d76e1f Mon Sep 17 00:00:00 2001 From: Stephen Morris Date: Wed, 7 Dec 2011 16:26:47 +0000 Subject: [PATCH] [1198] Changes as a result of review --- src/lib/datasrc/database.cc | 45 +++++++++++----------------- src/lib/datasrc/datasrc_messages.mes | 11 ++++--- src/lib/datasrc/zone.h | 15 ++++++---- tools/reorder_message_file.py | 10 +++---- 4 files changed, 36 insertions(+), 45 deletions(-) diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc index 91566b3fd0..dd88d51644 100644 --- a/src/lib/datasrc/database.cc +++ b/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 - // the location of the delegation cut when we encounter it here. - // TODO: where does it say we can't return wildcard glue? + // case, we return the delegation instead (see RFC 1034, section 4.3.3). + // To save a new search, we record the location of the delegation cut when + // 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). - // pretend something is here as well. + // not exist in the database, it does exist in the DNS tree). 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) { - // No match on a wildcard, so return the covering NSEC if DNSSEC - // data was requested. - return (FindResult(NXDOMAIN, findNSECCover(name))); + if (wresult.code != NXDOMAIN) { + return (FindResult(wresult.code, wresult.rrset)); } - 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). - // TODO: Why is DNAME ignored? + // apex - DNAME is ignored here as it redirects DNS names subordinate to + // 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 - // originated the query that caues this result. If so, it will restart - // the resolution process with the name that is the target of this - // CNAME.) First though, do a sanity check to ensure that there is - // 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. + // at the name we are searching so we return it. (The caller may + // want to continue the lookup by replacing the query name with the + // canonical name and the original RR type.) First though, do a sanity + // check to ensure that there is only one RR in the CNAME RRset. if (cni->second->getRdataCount() != 1) { isc_throw(DataSourceError, "CNAME with " << cni->second->getRdataCount() << " rdata at " << name << diff --git a/src/lib/datasrc/datasrc_messages.mes b/src/lib/datasrc/datasrc_messages.mes index 54fe79956b..f1066224b0 100644 --- a/src/lib/datasrc/datasrc_messages.mes +++ b/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 -though it was not the RR type being sought, it is returned. If the query -of the database was issued by a result searching for the name, the return of -the CNAME record will cause that resolver to issue another query for the -target of the CNAME. +When searching the domain for a name a CNAME was found at that name. +Even though it was not the RR type being sought, it is returned. (The +caller may want to continue the lookup by replacing the query name with +the canonical name and restarting the query with the original RR type.) % 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 diff --git a/src/lib/datasrc/zone.h b/src/lib/datasrc/zone.h index f8246360df..afe06d9d9b 100644 --- a/src/lib/datasrc/zone.h +++ b/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 - /// allocation, especially for constructing the resulting RRset, and may - /// throw an exception if it fails. - /// It throws DuplicateRRset exception if there are duplicate rrsets under - /// the same domain. - /// It should not throw other types of exceptions. + /// \exception std::bad_alloc Memory allocation such as for constructing + /// the resulting RRset fails + /// \exception DataSourceError Derived class specific exception, e.g. + /// when encountering a bad zone configuration or database connection + /// failure. Although these are considered rare, exceptional events, + /// 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. diff --git a/tools/reorder_message_file.py b/tools/reorder_message_file.py index 3f95544d01..31f4941131 100644 --- a/tools/reorder_message_file.py +++ b/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])