Opened 8 years ago

Closed 8 years ago

#1470 closed defect (fixed)

address trivial fixes from francis' mail

Reported by: jelte Owned by: stephen
Priority: medium Milestone: Sprint-20111220
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: none
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Francis created a big list of mostly trivial issues that should be straightforward to address (thank you!); see https://lists.isc.org/pipermail/bind10-dev/2011-November/002809.html

Some of these issues already have their own tickets (at least #903 and #911), and some issues may be not as trivial.

As a compromise between making 50 tickets for simple search&replaces, and having one ticket that is too big, I suggest we make this task to go through the list, address the easy ones, and create new tickets for those that might be more problematic.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111220

comment:3 Changed 8 years ago by shane

  • Feature Depending on Ticket set to none

comment:4 Changed 8 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:5 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

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

comment:6 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:7 in reply to: ↑ 5 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen
  • replace shared_ptr by boost::shared_ptr in:

<snip>

Implemented in delta from 0fd58479c441c0ce5584df6ab898594345e69ef5 to f1f4ce3e3014366d4916f924655c27761327c681

I've replaced a few more that I found, see commit
502cc85e6d101d89210cf8a1a41f06ea8b2412dd

  • *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

Do you know if this had a ticket too? (btw, should we add a comment to the added test that this is what it tests?)

  • 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.

Are there tickets for this yet?

  • *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.

I'd personally prefer exceptions that would result in a LOG_ERROR rather than asserts that result in abort(), but not sure whether we should check in the first place. Kinda depends on whether it would result in bad behaviour if it occurs (that we don't notice in our tests)

For the rest the changes all look fine

comment:8 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

Do you know if this had a ticket too?

On looking, it did - ticket #866. I will close that this when this one is closed.

(btw, should we add a comment to the added test that this is what it tests?)

No, none of the other tests are commented. Also, it's not really to check that - I had assumed that it was a Windows only bug had been relying on Francis to test it. (However, when I discovered #866, I rebuilt the code - both with and without the change - with _GLIBCXX_DEBUG defined: the bug appears with the old version of data.cc and goes away with the new version.)

The test itself is more to check that the order of the data in the maps does not matter - all other tests check removing "b, c" from "a, b, c" gives "a". This check tests that removing "c, b" from "a, b, c" still gives "a".

BTW, on checking after your comment I realised that I had only updated the test of the non-const version of removeIdentical(). I've updated the "const" version as well.

Are there tickets for this yet?

I don't know. There are TODOs all over the place in BIND 10 (on the latest version of master, I counted 154 in .cc files, 62 in .h files and 49 in .py files) so I'm not certain why this one should be special.

I'd personally prefer exceptions that would result in a LOG_ERROR rather than asserts that result in abort()...

I had a similar conversation with Jinmei on another ticket. I think he wanted to raise this at the F2F.

One more change to check please - no ChangeLog entry is planned as these are fixes invisible to the user.

comment:9 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

Ok, looks good, please go ahead and merge.

comment:10 Changed 8 years ago by stephen

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged in command ee21a81dbcfc6dc3587a9ca8ae95175c3b52bfea

#866 and #903 have also been closed as this fix addressed the issues raised by those tickets.

Note: See TracTickets for help on using tickets.