[trac764] review comments

src/lib/log/compiler/message.cc

@@ -56,12 +56,13 @@ static const char* VERSION = "1.0-0";
 /// \b Invocation<BR>
 /// The program is invoked with the command:
 ///
-/// <tt>message [-v | -h | -p | -d <dir> | \<message-file\>]</tt>
+/// <tt>message [-v | -h | -p | -d <dir> | <message-file>]</tt>
 ///
-/// It reads the message file and writes out two files of the same name in the
-/// default directory but with extensions of .h and .cc.
+/// It reads the message file and writes out two files of the same
+/// name in the current working directory (unless -d is used) but
+/// with extensions of .h and .cc.
 ///
-/// \-v causes it to print the version number and exit. \-h prints a help
+/// -v causes it to print the version number and exit. -h prints a help
 /// message (and exits). -p sets the output to python. -d <dir> will make
 /// it write the output file(s) to dir instead of current working
 /// directory

src/lib/python/isc/config/Makefile.am

@@ -7,10 +7,13 @@ pythondir = $(pyexecdir)/isc/config
 
 # Define rule to build logging source files from message file
 cfgmgr_messages.py: cfgmgr_messages.mes
-	$(top_builddir)/src/lib/log/compiler/message -p $(top_srcdir)/src/lib/python/isc/config/cfgmgr_messages.mes
+	$(top_builddir)/src/lib/log/compiler/message \
+	-p $(top_srcdir)/src/lib/python/isc/config/cfgmgr_messages.mes
 
 $(top_builddir)/src/lib/python/config_messages.py: config_messages.mes
-	$(top_builddir)/src/lib/log/compiler/message -p -d $(top_builddir)/src/lib/python $(top_srcdir)/src/lib/python/isc/config/config_messages.mes
+	$(top_builddir)/src/lib/log/compiler/message \
+		-p -d $(top_builddir)/src/lib/python \
+		$(top_srcdir)/src/lib/python/isc/config/config_messages.mes
 
 CLEANFILES =  cfgmgr_messages.py cfgmgr_messages.pyc
 CLEANFILES += $(top_builddir)/src/lib/python/config_messages.py

src/lib/python/isc/notify/Makefile.am

@@ -6,7 +6,9 @@ pyexec_DATA = $(top_builddir)/src/lib/python/notify_out_messages.py
 pythondir = $(pyexecdir)/isc/notify
 
 $(top_builddir)/src/lib/python/notify_out_messages.py: notify_out_messages.mes
-	$(top_builddir)/src/lib/log/compiler/message -p -d $(top_builddir)/src/lib/python $(top_srcdir)/src/lib/python/isc/notify/notify_out_messages.mes
+	$(top_builddir)/src/lib/log/compiler/message \
+		-p -d $(top_builddir)/src/lib/python \
+		$(top_srcdir)/src/lib/python/isc/notify/notify_out_messages.mes
 
 EXTRA_DIST = notify_out_messages.mes
 

src/lib/python/isc/notify/notify_out.py

@@ -30,7 +30,7 @@ logger = isc.log.Logger("notify_out")
 try:
     from pydnspp import *
 except ImportError as e:
-    logger.error(NOTIFY_OUT_IMPORT_DNS, str(e))
+    logger.error(NOTIFY_OUT_IMPORT_ERROR, str(e))
 
 ZONE_NEW_DATA_READY_CMD = 'zone_new_data_ready'
 _MAX_NOTIFY_NUM = 30
@@ -361,18 +361,19 @@ class NotifyOut:
         tgt = zone_notify_info.get_current_notify_target()
         if event_type == _EVENT_READ:
             reply = self._get_notify_reply(zone_notify_info.get_socket(), tgt)
-            if reply:
-                if self._handle_notify_reply(zone_notify_info, reply):
+            if reply is not None:
+                if self._handle_notify_reply(zone_notify_info, reply, tgt):
                     self._notify_next_target(zone_notify_info)
 
         elif event_type == _EVENT_TIMEOUT and zone_notify_info.notify_try_num > 0:
-            logger.info(NOTIFY_OUT_TIMEOUT_RETRY, tgt[0], tgt[1])
+            logger.info(NOTIFY_OUT_TIMEOUT, tgt[0], tgt[1])
 
         tgt = zone_notify_info.get_current_notify_target()
         if tgt:
             zone_notify_info.notify_try_num += 1
             if zone_notify_info.notify_try_num > _MAX_NOTIFY_TRY_NUM:
-                logger.info(NOTIFY_OUT_RETRY_EXCEEDED, tgt[0], tgt[1])
+                logger.error(NOTIFY_OUT_RETRY_EXCEEDED, tgt[0], tgt[1],
+                             _MAX_NOTIFY_TRY_NUM)
                 self._notify_next_target(zone_notify_info)
             else:
                 # set exponential backoff according rfc1996 section 3.6
@@ -452,34 +453,32 @@ class NotifyOut:
 
     def _handle_notify_reply(self, zone_notify_info, msg_data, from_addr):
         '''Parse the notify reply message.
-        TODO, the error message should be refined properly.
         rcode will not checked here, If we get the response
         from the slave, it means the slaves has got the notify.'''
         msg = Message(Message.PARSE)
         try:
-            #errstr = 'notify reply error: '
             msg.from_wire(msg_data)
             if not msg.get_header_flag(Message.HEADERFLAG_QR):
-                logger.error(NOTIFY_OUT_REPLY_QR_NOT_SET, from_addr[0],
-                             from_addr[1])
+                logger.warn(NOTIFY_OUT_REPLY_QR_NOT_SET, from_addr[0],
+                            from_addr[1])
                 return _BAD_QR
 
             if msg.get_qid() != zone_notify_info.notify_msg_id:
-                logger.error(NOTIFY_OUT_REPLY_BAD_QID, from_addr[0],
-                             from_addr[1], msg.get_qid(),
-                             zone_notify_info.notify_msg_id)
+                logger.warn(NOTIFY_OUT_REPLY_BAD_QID, from_addr[0],
+                            from_addr[1], msg.get_qid(),
+                            zone_notify_info.notify_msg_id)
                 return _BAD_QUERY_ID
 
             question = msg.get_question()[0]
             if question.get_name() != Name(zone_notify_info.zone_name):
-                logger.error(NOTIFY_OUT_REPLY_BAD_QUERY_NAME, from_addr[0],
-                             from_addr[1], question.get_name().to_text(),
-                             Name(zone_notify_info.zone_name).to_text())
+                logger.warn(NOTIFY_OUT_REPLY_BAD_QUERY_NAME, from_addr[0],
+                            from_addr[1], question.get_name().to_text(),
+                            Name(zone_notify_info.zone_name).to_text())
                 return _BAD_QUERY_NAME
 
             if msg.get_opcode() != Opcode.NOTIFY():
-                logger.error(NOTIFY_OUT_REPLY_BAD_OPCODE, from_addr[0],
-                             from_addr[1], msg.get_opcode().to_text())
+                logger.warn(NOTIFY_OUT_REPLY_BAD_OPCODE, from_addr[0],
+                            from_addr[1], msg.get_opcode().to_text())
                 return _BAD_OPCODE
         except Exception as err:
             # We don't care what exception, just report it?

src/lib/python/isc/notify/notify_out_messages.mes

@@ -15,7 +15,7 @@
 # No namespace declaration - these constants go in the global namespace
 # of the notify_out_messages python module.
 
-% NOTIFY_OUT_IMPORT error importing python module: %1
+% NOTIFY_OUT_IMPORT_ERROR error importing python module: %1
 There was an error importing a python module. One of the modules
 needed by notify_out could not be found. This suggests that either
 some libraries are missing on the system, or the PYTHONPATH variable
@@ -25,28 +25,35 @@ depends on your system and your specific installation.
 % NOTIFY_OUT_INVALID_ADDRESS invalid address %1#%2: %3
 The notify_out library tried to send a notify packet to the given
 address, but it appears to be an invalid address. The configuration
-for secondary nameservers might contain a typographic error.
+for secondary nameservers might contain a typographic error, or a
+different BIND 10 module has forgotten to validate its data before
+sending this module a notify command. As such, this should normally
+not happen, and points to an oversight in a different module.
 
 % NOTIFY_OUT_REPLY_BAD_OPCODE bad opcode in notify reply from %1#%2: %3
-The notify_out library sent a notify packet to the nameserver at the
-given address, but the response did not have the opcode set to NOTIFY.
-The opcode in the response is printed.
+The notify_out library sent a notify message to the nameserver at
+the given address, but the response did not have the opcode set to
+NOTIFY. The opcode in the response is printed. Since there was a
+response, no more notifies will be sent to this server for this
+notification event.
 
 % NOTIFY_OUT_REPLY_BAD_QID bad QID in notify reply from %1#%2: got %3, should be %4
-The notify_out library sent a notify packet to the nameserver at the
-given address, but the query id in the response does not match the one
-we sent.
+The notify_out library sent a notify message to the nameserver at
+the given address, but the query id in the response does not match
+the one we sent. Since there was a response, no more notifies will
+be sent to this server for this notification event.
 
 % NOTIFY_OUT_REPLY_BAD_QUERY_NAME bad query name in notify reply from %1#%2: got %3, should be %4
-The notify_out library sent a notify packet to the nameserver at the
-given address, but the query name in the response does not match the one
-we sent.
+The notify_out library sent a notify message to the nameserver at
+the given address, but the query name in the response does not match
+the one we sent. Since there was a response, no more notifies will
+be sent to this server for this notification event.
 
 % NOTIFY_OUT_REPLY_QR_NOT_SET QR flags set to 0 in reply to notify from %1#%2
 The notify_out library sent a notify packet to the namesever at the
 given address, but the reply did not have the QR bit set to one.
 
-% NOTIFY_OUT_RETRY_EXCEEDED notify to %1#%2: number of retries exceeded
+% NOTIFY_OUT_RETRY_EXCEEDED notify to %1#%2: number of retries (%3) exceeded
 The maximum number of retries for the notify target has been exceeded.
 Either the address of the secondary nameserver is wrong, or it is not
 responding.
@@ -56,22 +63,24 @@ A notify message is sent to the secondary nameserver at the given
 address.
 
 % NOTIFY_OUT_SOCKET_ERROR socket error sending notify to %1#%2: %3
-There was a network connection error while trying to send a notify
-packet to the given address. The socket error is printed and should
-provide more information.
+There was a network error while trying to send a notify packet to
+the given address. The address might be unreachable. The socket
+error is printed and should provide more information.
 
 % NOTIFY_OUT_SOCKET_RECV_ERROR socket error reading notify reply from %1#%2: %3
-There was a network connection error while trying to read a notify reply
+There was a network error while trying to read a notify reply
 packet from the given address. The socket error is printed and should
 provide more information.
 
-% NOTIFY_OUT_TIMEOUT_RETRY retry notify to %1#%2
+% NOTIFY_OUT_TIMEOUT retry notify to %1#%2
 The notify message to the given address (noted as address#port) has
-timed out, and the message is sent again.
+timed out, and the message will be resent until the max retry limit
+is reached.
 
 % NOTIFY_OUT_REPLY_UNCAUGHT_EXCEPTION uncaught exception: %1
 There was an uncaught exception in the handling of a notify reply
-packet. The error is printed, and notify_out will treat the response
-as a bad packet, but this does point to a programming error, since
-all exceptions should have been caught explicitely. Please file
-a bug report.
+packet, either in the packet parser, or while trying to extract data
+from the parsed packet. The error is printed, and notify_out will
+treat the response as a bad packet, but this does point to a
+programming error, since all exceptions should have been caught
+explicitely. Please file a bug report.

src/lib/python/isc/notify/tests/notify_out_test.py

@@ -301,6 +301,15 @@ class TestNotifyOut(unittest.TestCase):
         self._notify._zone_notify_handler(example_net_info, notify_out._EVENT_NONE)
         self.assertNotEqual(cur_tgt, example_net_info._notify_current)
 
+        cur_tgt = example_net_info._notify_current
+        example_net_info.create_socket('127.0.0.1')
+        # dns message, will result in bad_qid, but what we are testing
+        # here is whether handle_notify_reply is called correctly
+        example_net_info._sock.remote_end().send(b'\x2f\x18\xa0\x00\x00\x01\x00\x00\x00\x00\x00\x00\x07example\03com\x00\x00\x06\x00\x01')
+        self._notify._zone_notify_handler(example_net_info, notify_out._EVENT_READ)
+        self.assertNotEqual(cur_tgt, example_net_info._notify_current)
+
+
     def _example_net_data_reader(self):
         zone_data = [
         ('example.net.',         '1000',  'IN',  'SOA', 'a.dns.example.net. mail.example.net. 1 1 1 1 1'),

src/lib/util/filename.cc

@@ -134,14 +134,20 @@ Filename::useAsDefault(const string& name) const {
 
 void
 Filename::setDirectory(const std::string& new_directory) {
-    directory_ = new_directory;
-    // append '/' if necessary
-    size_t sep = directory_.rfind('/');
-    if (sep == std::string::npos || sep < directory_.size() - 1) {
-        directory_ += "/";
+    std::string directory(new_directory);
+
+    if (directory.length() > 0) {
+        // append '/' if necessary
+        size_t sep = directory.rfind('/');
+        if (sep == std::string::npos || sep < directory.size() - 1) {
+            directory += "/";
+        }
     }
     // and regenerate the full name
-    full_name_ = directory_ + name_ + extension_;
+    std::string full_name = directory + name_ + extension_;
+
+    directory_.swap(directory);
+    full_name_.swap(full_name);
 }
 
 

src/lib/util/filename.h

@@ -86,7 +86,11 @@ public:
         return (directory_);
     }
 
-    /// \return Set directory for the file
+    /// \brief Set directory for the file
+    ///
+    /// \param new_directory The directory to set. If this is an empty
+    ///        string, the directory this filename object currently
+    ///        has will be removed.
     void setDirectory(const std::string& new_directory);
 
     /// \return Name of Given File Name

src/lib/util/tests/filename_unittest.cc

@@ -200,9 +200,9 @@ TEST_F(FilenameTest, setDirectory) {
     EXPECT_EQ("/a.b", fname.expandWithDefault(""));
 
     fname.setDirectory("");
-    EXPECT_EQ("/", fname.directory());
-    EXPECT_EQ("/a.b", fname.fullName());
-    EXPECT_EQ("/a.b", fname.expandWithDefault(""));
+    EXPECT_EQ("", fname.directory());
+    EXPECT_EQ("a.b", fname.fullName());
+    EXPECT_EQ("a.b", fname.expandWithDefault(""));
 
     fname = Filename("/first/a.b");
     EXPECT_EQ("/first/", fname.directory());