Merge #2726

Enable more checking in cppcheck and fix the warnings produced by that.

Makefile.am

@@ -110,7 +110,8 @@ report-coverage: report-cpp-coverage report-python-coverage
 
 # for static C++ check using cppcheck (when available)
 cppcheck:
-	cppcheck --enable=all --suppressions src/cppcheck-suppress.lst --inline-suppr \
+	cppcheck -I./src/lib -I./src/bin --enable=all --suppressions \
+		src/cppcheck-suppress.lst --inline-suppr \
 		--quiet --error-exitcode=1 \
 		--template '{file}:{line}: check_fail: {message} ({severity},{id})' \
 		src

src/bin/auth/auth_srv.cc

@@ -388,7 +388,8 @@ private:
 };
 
 AuthSrv::AuthSrv(isc::xfr::AbstractXfroutClient& xfrout_client,
-                 isc::util::io::BaseSocketSessionForwarder& ddns_forwarder)
+                 isc::util::io::BaseSocketSessionForwarder& ddns_forwarder) :
+    dnss_(NULL)
 {
     impl_ = new AuthSrvImpl(xfrout_client, ddns_forwarder);
     checkin_ = new ConfigChecker(this);

src/bin/dhcp4/dhcp4_srv.cc

@@ -126,7 +126,8 @@ bool
 Dhcpv4Srv::run() {
     while (!shutdown_) {
         /// @todo: calculate actual timeout once we have lease database
-        int timeout = 1000;
+        //cppcheck-suppress variableScope This is temporary anyway
+        const int timeout = 1000;
 
         // client's message and server's response
         Pkt4Ptr query;

src/bin/dhcp6/dhcp6_srv.cc

@@ -134,7 +134,8 @@ bool Dhcpv6Srv::run() {
         /// For now, we are just calling select for 1000 seconds. There
         /// were some issues reported on some systems when calling select()
         /// with too large values. Unfortunately, I don't recall the details.
-        int timeout = 1000;
+        //cppcheck-suppress variableScope This is temporary anyway
+        const int timeout = 1000;
 
         // client's message and server's response
         Pkt6Ptr query;

src/lib/asiodns/io_fetch.cc

@@ -410,10 +410,9 @@ void IOFetch::logIOFailure(asio::error_code ec) {
            (data_->origin == ASIODNS_READ_DATA) ||
            (data_->origin == ASIODNS_UNKNOWN_ORIGIN));
 
-    static const char* PROTOCOL[2] = {"TCP", "UDP"};
     LOG_ERROR(logger, data_->origin).arg(ec.value()).
         arg((data_->remote_snd->getProtocol() == IPPROTO_TCP) ?
-                     PROTOCOL[0] : PROTOCOL[1]).
+                     "TCP" : "UDP").
         arg(data_->remote_snd->getAddress().toText()).
         arg(data_->remote_snd->getPort());
 }

src/lib/cc/data.cc

@@ -693,10 +693,9 @@ NullElement::toJSON(std::ostream& ss) const {
 void
 StringElement::toJSON(std::ostream& ss) const {
     ss << "\"";
-    char c;
     const std::string& str = stringValue();
     for (size_t i = 0; i < str.size(); ++i) {
-        c = str[i];
+        const char c = str[i];
         // Escape characters as defined in JSON spec
         // Note that we do not escape forward slash; this
         // is allowed, but not mandatory.

src/lib/datasrc/memory/domaintree.h

@@ -705,7 +705,13 @@ public:
                         // XXX: meaningless initial values:
                         last_comparison_(0, 0,
                                          isc::dns::NameComparisonResult::EQUAL)
-    {}
+    {
+        // To silence cppcheck. We don't really use the values before
+        // initialization, but this is cleaner anyway.
+        for (size_t i = 0; i < RBT_MAX_LEVEL; ++i) {
+            nodes_[i] = NULL;
+        }
+    }
 
     /// \brief Copy constructor.
     ///
@@ -828,6 +834,7 @@ private:
     /// the top node
     ///
     /// \exception None
+    // cppcheck-suppress unusedPrivateFunction (false positive, it is used)
     void pop() {
         assert(!isEmpty());
         --level_count_;
@@ -840,6 +847,7 @@ private:
     /// otherwise the node should be the root node of DomainTree.
     ///
     /// \exception None
+    // cppcheck-suppress unusedPrivateFunction (false positive, it is used)
     void push(const DomainTreeNode<T>* node) {
         assert(level_count_ < RBT_MAX_LEVEL);
         nodes_[level_count_++] = node;

src/lib/datasrc/tests/database_unittest.cc

@@ -1137,6 +1137,9 @@ const char* TEST_NSEC3_RECORDS[][5] = {
 };
 
 DatabaseClientTest::DatabaseClientTest() :
+    // We need to initialize to something, and not being mock is safer
+    // until we know for sure.
+    is_mock_(false),
     zname_("example.org"), qname_("www.example.org"),
     qclass_(dns::RRClass::IN()),
     qtype_(dns::RRType::A()),

src/lib/datasrc/tests/factory_unittest.cc

@@ -39,6 +39,8 @@ void
 pathtestHelper(const std::string& file, const std::string& expected_error) {
     std::string error;
     try {
+        // cppcheck-suppress unusedScopedObject We just check if it throws
+        // to create, not use it. That's OK.
         DataSourceClientContainer(file, ElementPtr());
     } catch (const DataSourceLibraryError& dsle) {
         error = dsle.what();

src/lib/dhcp/option_custom.h

@@ -310,6 +310,7 @@ private:
     ///
     /// @throw isc::dhcp::InvalidDataType if the type is invalid.
     template<typename T>
+    // cppcheck-suppress unusedPrivateFunction
     void checkDataType(const uint32_t index) const;
 
     /// @brief Check if data field index is valid.

src/lib/dhcp/option_definition.cc

@@ -228,14 +228,6 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
 }
 
 void
-OptionDefinition::sanityCheckUniverse(const Option::Universe expected_universe,
-                                      const Option::Universe actual_universe) {
-    if (expected_universe != actual_universe) {
-        isc_throw(isc::BadValue, "invalid universe specified for the option");
-    }
-}
-
-void
 OptionDefinition::validate() const {
 
     using namespace boost::algorithm;

src/lib/dhcp/option_definition.h

@@ -501,15 +501,6 @@ private:
     void writeToBuffer(const std::string& value, const OptionDataType type,
                        OptionBuffer& buf) const;
 
-    /// @brief Sanity check universe value.
-    ///
-    /// @param expected_universe expected universe value.
-    /// @param actual_universe actual universe value.
-    ///
-    /// @throw isc::BadValue if expected universe and actual universe don't match.
-   static inline void sanityCheckUniverse(const Option::Universe expected_universe,
-                                          const Option::Universe actual_universe);
-
     /// Option name.
     std::string name_;
     /// Option code.

src/lib/dns/master_lexer.h

@@ -22,6 +22,8 @@
 
 #include <stdint.h>
 
+#include <boost/noncopyable.hpp>
+
 namespace isc {
 namespace dns {
 namespace master_lexer_internal {
@@ -303,7 +305,7 @@ private:
 /// implementation of the exception handling).  For these reasons, some of
 /// this class does not throw for an error that would be reported as an
 /// exception in other classes.
-class MasterLexer {
+class MasterLexer : public boost::noncopyable {
     friend class master_lexer_internal::State;
 public:
     /// \brief Exception thrown when we fail to read from the input

src/lib/dns/python/pydnspp_common.cc

@@ -56,9 +56,8 @@ PyObject* po_DNSMessageBADVERS;
 
 int
 readDataFromSequence(uint8_t *data, size_t len, PyObject* sequence) {
-    PyObject* el = NULL;
     for (size_t i = 0; i < len; i++) {
-        el = PySequence_GetItem(sequence, i);
+        PyObject *el = PySequence_GetItem(sequence, i);
         if (!el) {
             PyErr_SetString(PyExc_TypeError,
                 "sequence too short");

src/lib/dns/rdata.cc

@@ -310,6 +310,9 @@ Generic::Generic(const Generic& source) :
 {}
 
 Generic&
+// Our check is better than the usual if (this == &source),
+// but cppcheck doesn't recognize it.
+// cppcheck-suppress operatorEqToSelf
 Generic::operator=(const Generic& source) {
     if (impl_ == source.impl_) {
         return (*this);

src/lib/dns/rdata/any_255/tsig_250.cc

@@ -338,7 +338,7 @@ TSIG::TSIG(const TSIG& source) : Rdata(), impl_(new TSIGImpl(*source.impl_))
 
 TSIG&
 TSIG::operator=(const TSIG& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/detail/ds_like.h

@@ -188,12 +188,12 @@ public:
     /// \brief The copy constructor.
     ///
     /// Trivial for now, we could've used the default one.
-    DSLikeImpl(const DSLikeImpl& source) {
-        digest_ = source.digest_;
-        tag_ = source.tag_;
-        algorithm_ = source.algorithm_;
-        digest_type_ = source.digest_type_;
-    }
+    DSLikeImpl(const DSLikeImpl& source) :
+        tag_(source.tag_),
+        algorithm_(source.algorithm_),
+        digest_type_(source.digest_type_),
+        digest_(source.digest_)
+    {}
 
     /// \brief Convert the DS-like data to a string.
     ///

src/lib/dns/rdata/generic/dlv_32769.cc

@@ -62,7 +62,7 @@ DLV::DLV(const DLV& source) :
 /// PIMPL-induced logic
 DLV&
 DLV::operator=(const DLV& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/dnskey_48.cc

@@ -212,7 +212,7 @@ DNSKEY::DNSKEY(const DNSKEY& source) :
 
 DNSKEY&
 DNSKEY::operator=(const DNSKEY& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/ds_43.cc

@@ -50,7 +50,7 @@ DS::DS(const DS& source) :
 
 DS&
 DS::operator=(const DS& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/nsec3_50.cc

@@ -200,7 +200,7 @@ NSEC3::NSEC3(const NSEC3& source) :
 
 NSEC3&
 NSEC3::operator=(const NSEC3& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/nsec3param_51.cc

@@ -139,7 +139,7 @@ NSEC3PARAM::NSEC3PARAM(const NSEC3PARAM& source) :
 
 NSEC3PARAM&
 NSEC3PARAM::operator=(const NSEC3PARAM& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/nsec_47.cc

@@ -155,7 +155,7 @@ NSEC::NSEC(const NSEC& source) :
 
 NSEC&
 NSEC::operator=(const NSEC& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/rrsig_46.cc

@@ -230,7 +230,7 @@ RRSIG::RRSIG(const RRSIG& source) :
 
 RRSIG&
 RRSIG::operator=(const RRSIG& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/spf_99.cc

@@ -42,7 +42,7 @@ using namespace isc::util;
 /// This method never throws an exception otherwise.
 SPF&
 SPF::operator=(const SPF& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/sshfp_44.cc

@@ -199,7 +199,7 @@ SSHFP::SSHFP(const SSHFP& other) :
 
 SSHFP&
 SSHFP::operator=(const SSHFP& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/generic/txt_16.cc

@@ -33,7 +33,7 @@ using namespace isc::util;
 
 TXT&
 TXT::operator=(const TXT& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rdata/in_1/srv_33.cc

@@ -190,7 +190,7 @@ SRV::SRV(const SRV& source) :
 
 SRV&
 SRV::operator=(const SRV& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/dns/rrttl.cc

@@ -74,10 +74,10 @@ parseTTLString(const string& ttlstr, uint32_t& ttlval, string* error_txt) {
     const string::const_iterator end = ttlstr.end();
     string::const_iterator pos = ttlstr.begin();
 
-    // When we detect we have some units
-    bool units_mode = false;
-
     try {
+        // When we detect we have some units
+        bool units_mode = false;
+
         while (pos != end) {
             // Find the first unit, if there's any.
             const string::const_iterator unit = find_if(pos, end, myIsalpha);

src/lib/dns/serial.h

@@ -59,7 +59,10 @@ public:
     /// \brief Direct assignment from other Serial
     ///
     /// \param other The Serial to assign the value from
-    void operator=(const Serial& other) { value_ = other.getValue(); }
+    Serial& operator=(const Serial& other) {
+        value_ = other.getValue();
+        return (*this);
+    }
 
     /// \brief Direct assignment from value
     ///

src/lib/dns/tsigkey.cc

@@ -148,7 +148,7 @@ TSIGKey::TSIGKey(const TSIGKey& source) : impl_(new TSIGKeyImpl(*source.impl_))
 
 TSIGKey&
 TSIGKey::operator=(const TSIGKey& source) {
-    if (impl_ == source.impl_) {
+    if (this == &source) {
         return (*this);
     }
 

src/lib/log/logger_impl.h

@@ -23,6 +23,7 @@
 #include <string>
 #include <map>
 #include <utility>
+#include <boost/noncopyable.hpp>
 
 
 // log4cplus logger header file
@@ -61,7 +62,7 @@ namespace log {
 /// b) The idea of debug levels is implemented.  See logger_level.h and
 /// logger_level_impl.h for more details on this.
 
-class LoggerImpl {
+class LoggerImpl : public boost::noncopyable {
 public:
 
     /// \brief Constructor

src/lib/log/logger_manager.h

@@ -19,6 +19,8 @@
 #include <util/threads/sync.h>
 #include <log/logger_specification.h>
 
+#include <boost/noncopyable.hpp>
+
 // Generated if, when updating the logging specification, an unknown
 // destination is encountered.
 class UnknownLoggingDestination : public isc::Exception {
@@ -41,7 +43,7 @@ class LoggerManagerImpl;
 /// To isolate the underlying implementation from basic processing, the
 /// LoggerManager is implemented using the "pimpl" idiom.
 
-class LoggerManager {
+class LoggerManager : public boost::noncopyable {
 public:
     /// \brief Constructor
     LoggerManager();

src/lib/log/message_exception.h

@@ -62,7 +62,7 @@ public:
     /// \param lineno Line number on which error occurred (if > 0).
     MessageException(const char* file, size_t line, const char* what,
                      MessageID id, const std::string& arg1, int lineno)
-        : isc::Exception(file, line, what), id_(id)
+        : isc::Exception(file, line, what), id_(id), lineno_(lineno)
     {
         if (lineno > 0) {
             args_.push_back(boost::lexical_cast<std::string>(lineno));
@@ -82,7 +82,7 @@ public:
     MessageException(const char* file, size_t line, const char *what,
                      MessageID id, const std::string& arg1,
                      const std::string& arg2, int lineno)
-        : isc::Exception(file, line, what), id_(id)
+        : isc::Exception(file, line, what), id_(id), lineno_(lineno)
     {
         if (lineno > 0) {
             args_.push_back(boost::lexical_cast<std::string>(lineno));

src/lib/python/isc/datasrc/configurableclientlist_python.cc

@@ -162,7 +162,7 @@ ConfigurableClientList_find(PyObject* po_self, PyObject* args) {
         int want_finder = 1;
         if (PyArg_ParseTuple(args, "O!|ii", &isc::dns::python::name_type,
                              &name_obj, &want_exact_match, &want_finder)) {
-            const isc::dns::Name
+            const isc::dns::Name&
                 name(isc::dns::python::PyName_ToName(name_obj));
             const ClientList::FindResult
                 result(self->cppobj->find(name, want_exact_match,

src/lib/resolve/recursive_query.cc

@@ -245,11 +245,6 @@ private:
 
     // normal query state
 
-    // Update the question that will be sent to the server
-    void setQuestion(const Question& new_question) {
-        question_ = new_question;
-    }
-
     // TODO: replace by our wrapper
     asio::deadline_timer client_timer;
     asio::deadline_timer lookup_timer;

src/lib/server_common/portconfig.cc

@@ -49,6 +49,9 @@ parseAddresses(isc::data::ConstElementPtr addresses,
                         "address and port");
                 }
                 try {
+                    // We create an IOAddress object to just check that
+                    // construction passes. It is immediately destroyed.
+                    // cppcheck-suppress unusedScopedObject
                     IOAddress(addr->stringValue());
                     if (port->intValue() < 0 ||
                         port->intValue() > 0xffff) {

src/lib/statistics/counter.h

@@ -55,7 +55,7 @@ public:
     /// \param type %Counter item to increment
     ///
     /// \throw isc::OutOfRange \a type is invalid
-    void inc(const Counter::Type type) {
+    void inc(const Counter::Type& type) {
         if (type >= counters_.size()) {
             isc_throw(isc::OutOfRange, "Counter type is out of range");
         }
@@ -68,7 +68,7 @@ public:
     /// \param type %Counter item to get the value of
     ///
     /// \throw isc::OutOfRange \a type is invalid
-    const Counter::Value& get(const Counter::Type type) const {
+    const Counter::Value& get(const Counter::Type& type) const {
         if (type >= counters_.size()) {
             isc_throw(isc::OutOfRange, "Counter type is out of range");
         }

src/lib/testutils/mockups.h

@@ -115,6 +115,8 @@ private:
 // to addServerXXX methods so the test code subsequently checks the parameters.
 class MockDNSService : public isc::asiodns::DNSServiceBase {
 public:
+    MockDNSService() : tcp_recv_timeout_(0) {}
+
     // A helper tuple of parameters passed to addServerUDPFromFD().
     struct UDPFdParams {
         int fd;

src/lib/util/buffer.h

@@ -342,15 +342,17 @@ public:
 
     /// \brief Assignment operator
     OutputBuffer& operator =(const OutputBuffer& other) {
-        uint8_t* newbuff(static_cast<uint8_t*>(malloc(other.allocated_)));
-        if (newbuff == NULL && other.allocated_ != 0) {
-            throw std::bad_alloc();
+        if (this != &other) {
+            uint8_t* newbuff(static_cast<uint8_t*>(malloc(other.allocated_)));
+            if (newbuff == NULL && other.allocated_ != 0) {
+                throw std::bad_alloc();
+            }
+            free(buffer_);
+            buffer_ = newbuff;
+            size_ = other.size_;
+            allocated_ = other.allocated_;
+            std::memcpy(buffer_, other.buffer_, size_);
         }
-        free(buffer_);
-        buffer_ = newbuff;
-        size_ = other.size_;
-        allocated_ = other.allocated_;
-        std::memcpy(buffer_, other.buffer_, size_);
         return (*this);
     }
 

src/lib/util/tests/buffer_unittest.cc

@@ -248,6 +248,11 @@ TEST_F(BufferTest, outputBufferAssign) {
     });
 }
 
+// Check assign to self doesn't break stuff
+TEST_F(BufferTest, outputBufferAssignSelf) {
+    EXPECT_NO_THROW(obuffer = obuffer);
+}
+
 TEST_F(BufferTest, outputBufferZeroSize) {
     // Some OSes might return NULL on malloc for 0 size, so check it works
     EXPECT_NO_THROW({

src/lib/util/unittests/mock_socketsession.h

@@ -42,7 +42,12 @@ class MockSocketSessionForwarder :
 public:
     MockSocketSessionForwarder() :
         is_connected_(false), connect_ok_(true), push_ok_(true),
-        close_ok_(true)
+        close_ok_(true),
+        // These are not used until set, but we set them anyway here,
+        // partly to silence cppcheck, and partly to be cleaner. Put some
+        // invalid values in.
+        pushed_sock_(-1), pushed_family_(-1), pushed_type_(-1),
+        pushed_protocol_(-1)
     {}
 
     virtual void connectToReceiver() {