BIND 10 #1470: address trivial fixes from francis' mail

BIND 10 Development do-not-reply at isc.org
Tue Dec 13 17:07:38 UTC 2011 

#1470: address trivial fixes from francis' mail
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  UnAssigned
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:  major  |  Sprint-20111220
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  none   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => UnAssigned
 * status:  assigned => reviewing


Comment:

 Given that there are a lot of changes, each involving a number of files,
 it might be easiest to review the individual deltas (although bear in mind
 that, for various reasons, the changes were not made in the order that
 they appear in the email).

 > These are from the WIN32 port effort:
 >  - src/bin/host/host.cc should use util/time_utilities.h
 gettimeWrapper()
 >   in place of the not portable gettimeofday()
 Not done here, this is the subject of ticket #911.  gettimeWrapper() just
 returns the "seconds" value in the tv_sec field; gettime() is used in both
 host.cc and in src/lib/resolve/recursive_query.cc where the fractional
 component is used. I suggest that we really need to create a set of
 operating-system wrapper functions, this being the first one.


 >  - replace shared_ptr by boost::shared_ptr in:
 >   * src/bin/resolver/resolver.cc
 >   * src/lib/acl/dns.cc
 >   * src/lib/acl/tests/loader_test.cc
 >   * src/lib/acl/tests/logic_check_test.cc
 >   * src/lib/datasrc/database.cc
 >   * src/lib/datasrc/tests/database_unittest.cc
 >   * src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
 >   * src/lib/dns/rrparamregistry-placeholder.cc
 >   * src/lib/nsas/tests/hash_table_unittest.cc
 >   * src/lib/python/isc/acl/dns_requestloader_python.cc
 Implemented in delta from 0fd58479c441c0ce5584df6ab898594345e69ef5 to
 f1f4ce3e3014366d4916f924655c27761327c681

 >  - include config.h first in:
 >   * src/bin/resolver/tests/response_scrubber_unittest.cc
 >   * src/lib/asiodns/dns_service.cc
 >   * src/lib/asiolink/io_service.cc
 >   * src/lib/nsas/tests/nameserver_address_store_unittest.cc
 >   * src/lib/resolve/recursive_query.cc
 Implemented in delta from commit f1f4ce3e3014366d4916f924655c27761327c681
 to commit f1104654a25beae9f1b6fcd7e09c5128346a150a

 >  - src/lib/acl/tests/ip_check_unittest.cc should use an unsigned type
 >   (vs. char) for expected_data initialized from values > 127
 Implemented in delta from commit c254f7fcb4fac6b47cc880221aa5d28a0772b641
 to commit 0910b0f180332e46fd0bac7867c646f76e040615

 >  - in src/lib/asiodns/dns_lookup.h replace the incorrect
 >      DNSLookup() : self_(this) {}
 >    by correct
 >      DNSLookup() { self_ = this; }
 >
 >  - in src/lib/asiodns/dns_server.h replace the incorrect
 >      DNSServer() : self_(this) {}
 >    by correct
 >      DNSServer() { self_ = this; }
 The following additional files had the same problem (reference to "this"
 in initializer list) and were also modified:
 * src/bin/auth/auth_srv.cc
 * src/bin/resolver/resolver/cc
 * src/lib/asiolink/simple_callback.h
 * src/lib/datasrc/rbtree.h
 * src/lib/resolve/recursive_query.h
 Implemented in change from 1615a521d61a0f47a627f92cc4f95d982a04f8d1 to
 c254f7fcb4fac6b47cc880221aa5d28a0772b641


 >  - in src/lib/asiodns/io_fetch.{h,cc} choose between struct and class
 >   for IOFetchData
 "struct" chosen.

 Implemented in delta from commit 0910b0f180332e46fd0bac7867c646f76e040615
 to commit 0daa6e10d2b7446069b3e43b8cbf80691270431f

 >  - qualify error_code into asio::error_code in:
 >   * src/lib/asiodns/tcp_server.cc
 >   * src/lib/asiodns/tests/io_fetch_unittest.cc
 >   * src/lib/asiodns/udp_server.cc
 >   * src/lib/asiolink/io_address.cc
 >   * src/lib/resolve/tests/recursive_query_unittest_2.cc
 Implemented in change from 0daa6e10d2b7446069b3e43b8cbf80691270431f to
 29003e06a65e9cbd18f83d44e197e70542a2a6a2

 >  - move to unsigned for loop index in:
 >   * src/lib/asiodns/tests/dns_server_unittest.cc (line 433)
 >   * src/lib/bench/benchmark.h (lines 123 (comment) and 264)
 >   * src/lib/cache/resolver_cache.cc (lines 167, 174 and 264)
 >   * src/lib/cc/data.cc (line 787, and change the type of s line 783 too)
 >   * src/lib/cc/session.cc (line 370)
 >   * src/lib/cryptolink/tests/crypto_unittests.cc (line 395)
 >   * src/lib/dns/rdata/generic/dnskey_48.cc (line 184, type size_t)
 >   * src/lib/dns/rdata/generic/nsec3_50.cc (line 224 and perhaps others)
 >   * src/lib/dns/rdata/generic/nsec_47.cc (line 146 and        perhaps
 others)
 >   * src/lib/dns/rdatafields.cc (lines 174 and 201)
 >   * src/lib/dns/tests/unittest_util.cc (lines 119 and 142)
 >   * src/lib/log/compiler/message.cc (line 227)
 >   * src/lib/log/logger_manager.cc (line 170)
 >   * src/lib/log/tests/logger_manager_unittest.cc (line 318)
 >   * src/lib/nsas/hash_key.cc (line 38)
 >   * src/lib/resolve/response_classifier.cc (lines 122, 164, 176 and 214)
 >   * src/lib/resolve/tests/recursive_query_unittest_2.cc (line 692)
 >   * src/lib/util/tests/base32hex_unittest.cc (line 157, type uint8_t)
 >   * src/lib/util/tests/hex_unittest.cc (line 113, type uint8_t)
 >   * src/lib/util/time_utilities.cc (line 161)
 Disagree for the need to make the change in the comment in benchmark.h at
 line 123 - "data"_set is a set<int>, so the insert operation should take
 an int as an argument. (The change at line 264 is correct though.)

 Implemented in delta from commit 29003e06a65e9cbd18f83d44e197e70542a2a6a2
 to commit bf2e60ca2a5bd14d057c5ad292b752e980d9637b

 >  - in src/lib/asiodns/udp_server.{h,c} choose between struct and class
 >   for Data
 "struct" chosen.

 Implemented in delta from commit bf2e60ca2a5bd14d057c5ad292b752e980d9637b
 to commit 62463d1d0236e7fb6c3bfc94b3b66e46c875ca93

 >
 >  - in src/lib/asiolink/simple_callback.h replace the incorrect
 >      !SimpleCallback() : self_(this) {}
 >    by correct
 >      !SimpleCallback() { self_ = this; }
 Tackled in the same set of changes in which DNSLookup and DNSServer were
 corrected.

 >  - remove the unused exception variable (i.e., keep only the type) in:
 >   * src/lib/asiolink/tcp_socket.h (line 279)
 >   * src/lib/bench/benchmark_util.cc (line 106)
 >   * src/lib/cc/data.cc (line 745)
 >   * src/lib/dhcp/libdhcp.cc (line 132)
 >   * src/lib/python/isc/log/log.cc (lines 240 and 243)
 >   * src/lib/server_common/portconfig.cc (line 62)
 >   * src/lib/util/strutil.h (line 194)
 Implemented in delta from commit 62463d1d0236e7fb6c3bfc94b3b66e46c875ca93
 to commit 0213d987ac8b4fb30bc1aa1ee6bd67cdfde02ce0

 >  - ***BUG*** in src/lib/cc/data.cc (first) removeIdentical() the
 >   remove() can make the it iterator pointing to the end() so
 >   the ++it is invalid
 >   (I already signaled this bug, please fix it! BTW, what is the
 >    escalation procedure?)
 Fix implemented in the change from commit
 0213d987ac8b4fb30bc1aa1ee6bd67cdfde02ce0 to commit
 f18114000f75a0615ae97e36c54881754cc84be9

 >  - in src/lib/datasrc/tests/database_unittest.cc change the type of
 >   cur_record_ to size_t
 Implemented in delta from commit f18114000f75a0615ae97e36c54881754cc84be9
 to commit 5cdcda0439416cbe1610a5990d30441c20e571eb

 >  - in src/lib/datasrc/tests/database_unittest.cc change the confusing
 >   initialization of for index name to a more standard style, i.e.:
 >   name(positive_names) -> name = positive_names
 >   (same for negative_names and negative_dnssec_names)
 >
 >  - same for src/lib/datasrc/tests/memory_datasrc_unittest.cc and
 >   the four name(names) -> name = names
 Both sets of modifications implemented in delta from commit
 5cdcda0439416cbe1610a5990d30441c20e571eb to commit
 c4ca960f3139817c9d3baf35c2b975a240c8acd2

 >  - ***BUG*** in src/lib/dns/masterload.cc masterLoad must check if
 >   the filename is not NULL before opening it
 Implemented in delta from commit c4ca960f3139817c9d3baf35c2b975a240c8acd2
 to commit 5dbdcfa0a3fdeab7b8757a8a22ff6d5b9cf87cbb

 >  - in src/lib/dns/message.cc revisit the counts_ definition
 >   (i.e., open a ticket for it so it will be done before the end
 >    of the year)
 Not done - there are any number of TODOs in the code that need addressing
 and this is just one of them.

 >  - in src/lib/dns/messagerenderer.cc the variable i line 120 should
 >   be at least unsigned (IMHO it should be size_t, i.e., same type than
 >   Name::MAX_WIRE)
 Implemented in delta from commit 5dbdcfa0a3fdeab7b8757a8a22ff6d5b9cf87cbb
 to cd4c9eb0868200fbd53beef6eebc35ba6982a798

 >  - in src/lib/dns/python/pydnspp_common.cc pydnspp_common.h is included
 >   twice, IMHO the first one (with <>) should be removed
 Implemented in delta from commit cd4c9eb0868200fbd53beef6eebc35ba6982a798
 to commit 0a5f810dd86794befad38590f68d3725828379eb

 >  - ***BUGS*** in src/lib/dns/rdata.cc there are two out of bound
 >   accesses with empty vectors:
 >    * line 115 where
 >     buffer.readData(&data[0], rdata_len);
 >    must be guard against rdata_len == 0
 >    * line 245 where
 >     if ((cmp = memcmp(&lhs.data_[0], &rhs.data_[0], len))
 >    must be guard against len == 0
 Implemented in delta from commit 0a5f810dd86794befad38590f68d3725828379eb
 to commit f15d8831381af8a56c62e5e8a762166fcd053422.

 However, should these be assertions?  The Rdata class is apparently
 protected against zero length strings, so arguably if "len" is zero, there
 is a programming error.

 >  - in src/lib/dns/tests/message_unittest.cc expected_mac should be
 >   at least unsigned (vs. char) as initialized with values > 127
 Implemented in delta from commit f15d8831381af8a56c62e5e8a762166fcd053422
 to commit b5f53a509974185553f40022b947c9c515992493

 >  - in src/lib/log/{logger_manager_impl,output_option}.h choose between
 >   struct and class for OutputOption
 "struct" chosen.

 Implemented in delta from commit b5f53a509974185553f40022b947c9c515992493
 to commit b5f53a509974185553f40022b947c9c515992493

 >  - in src/lib/log/logger_specification.h getAdditive() should return
 >   a bool (vs. an int)
 Implemented in delta from commit ae4e8ba0c81e273b515147e916e28924c2804c14
 to commit c066e58bd521a107d027818f47268c5183ec5b18

 >  - ***BUG*** src/lib/util/io/fd.cc was not yet fixed!!!
 Fixed in delta from commit f1104654a25beae9f1b6fcd7e09c5128346a150a to
 commit 3f83fac07d34fc709aebe528028f041fc3637bab

 >  - in src/lib/util/strutil.cc the 'Index into argument array' line 123
 >   should be at least unsigned
 Implemented in delta from commit c066e58bd521a107d027818f47268c5183ec5b18
 to commit 0fd58479c441c0ce5584df6ab898594345e69ef5

-- 
Ticket URL: <http://bind10.isc.org/ticket/1470#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development

More information about the bind10-tickets mailing list