[3806] Addressed some of the review comments.

Except user guide related and "fixme" leftovers.

src/bin/dhcp4/dhcp4_log.h

@@ -46,7 +46,7 @@ const int DBG_DHCP4_HOOKS = DBGLVL_TRACE_BASIC;
 /// @brief Debug level used to log the traces with some basic data.
 ///
 /// The basic data includes summary information, e.g. summary of the
-/// information returned by the particular function. It may also include
+/// information returned by a particular function. It may also include
 /// more detailed information in cases when it is warranted and the
 /// extraction of the data doesn't impact the server's performance
 /// significantly.

src/bin/dhcp4/dhcp4_messages.mes

@@ -25,7 +25,7 @@ occurred during this attempt. The reason for the error is included in
 the message.
 
 % DHCP4_BUFFER_RECEIVED received buffer from %1:%2 to %3:%4 over interface %5
-This debug message is logged when the server has received some packet
+This debug message is logged when the server has received a packet
 over the socket. When the message is logged the contents of the received
 packet hasn't been parsed yet. The only available information is the
 interface and the source and destination addresses/ports.
@@ -72,11 +72,11 @@ information. The second argument includes all classes to which the
 packet has been assigned.
 
 % DHCP4_CLIENT_FQDN_PROCESS %1: processing Client FQDN option
-This debug message is issued when the server starts to process the Client
+This debug message is issued when the server starts processing the Client
 FQDN option sent in the client's query. The argument includes the
 client and transaction identification information.
 
-% DHCP4_CLIENT_FQDN_DATA %1: Client FQDN option information: %2
+% DHCP4_CLIENT_FQDN_DATA %1: Client sent FQDN option: %2
 This debug message includes the detailed information extracted from the
 Client FQDN option sent in the query. The first argument includes the
 client and transaction identification information. The second argument
@@ -84,11 +84,11 @@ specifies the detailed information about the FQDN option received
 by the server.
 
 % DHCP4_CLIENT_HOSTNAME_PROCESS %1: processing client's Hostname option
-This debug message is issued when the server starts to process the Hostname
+This debug message is issued when the server starts processing the Hostname
 option sent in the client's query. The argument includes the client and
 transaction identification information.
 
-% DHCP4_CLIENT_HOSTNAME_DATA %1: client Hostname option information: %2
+% DHCP4_CLIENT_HOSTNAME_DATA %1: client sent Hostname option: %2
 This debug message includes the detailed information extracted from the
 Hostname option sent in the query. The first argument includes the
 client and transaction identification information. The second argument
@@ -226,11 +226,12 @@ client class has reported a failure. The response packet will not be sent.
 The argument specifies the client and the transaction identification
 information.
 
-% DHCP4_INFORM_DIRECT_REPLY %1: DHCPACK in reply to the DHCPINFORM will be sent directly to %2
+% DHCP4_INFORM_DIRECT_REPLY %1: DHCPACK in reply to the DHCPINFORM will be sent directly to %2 over %3
 This debug message is issued when the DHCPACK will be sent directly to the
 client, rather than via a relay. The first argument contains the client
 and transaction identification information. The second argument contains
-the client's address to which the response will be sent.
+the client's address to which the response will be sent. The third argument
+contains the local interface name.
 
 % DHCP4_INIT_FAIL failed to initialize Kea server: %1
 The server has failed to initialize. This may be because the configuration
@@ -541,7 +542,7 @@ client class has reported a failure. The response packet will not be sent.
 The argument contains the client and transaction identification
 information.
 
-% DHCP4_RESPONSE_DATA %1: responding with packet %2 (type %3), packet details: "%4"
+% DHCP4_RESPONSE_DATA %1: responding with packet %2 (type %3), packet details: %4
 A debug message including the detailed data about the packet being sent
 to the client. The first argument contains the client and the transaction
 identification information. The second and third argument contains the

src/bin/dhcp4/dhcp4_srv.cc

@@ -194,7 +194,6 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const bool use_bcast,
       use_bcast_(use_bcast), hook_index_pkt4_receive_(-1),
       hook_index_subnet4_select_(-1), hook_index_pkt4_send_(-1) {
 
-    // fixme: either remove or change its name.
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_OPEN_SOCKET).arg(port);
     try {
         // Port 0 is used for testing purposes where we don't open broadcast
@@ -1791,7 +1790,8 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
     if (ack->getRemoteAddr() != inform->getGiaddr()) {
         LOG_DEBUG(packet_logger, DBG_DHCP4_DETAIL, DHCP4_INFORM_DIRECT_REPLY)
             .arg(inform->getLabel())
-            .arg(ack->getRemoteAddr());
+            .arg(ack->getRemoteAddr())
+            .arg(ack->getIface());
         ack->setHops(0);
         ack->setGiaddr(IOAddress::IPV4_ZERO_ADDRESS());
         ack->delOption(DHO_DHCP_AGENT_OPTIONS);

src/lib/dhcp/option_int_array.h

@@ -262,7 +262,7 @@ public:
         std::string data_type = OptionDataTypeUtil::getDataTypeName(OptionDataTypeTraits<T>::type);
         for (typename std::vector<T>::const_iterator value = values_.begin();
              value != values_.end(); ++value) {
-            output << " " << *value << " (" << data_type << ")";
+            output << " " << *value << "(" << data_type << ")";
         }
 
         return (output.str());

src/lib/dhcp/pkt4.cc

@@ -362,8 +362,21 @@ Pkt4::toText() const {
     stringstream output;
     output << "local_address=" << local_addr_ << ":" << local_port_
         << ", remote_adress=" << remote_addr_
-        << ":" << remote_port_ << ", msg_type=" << static_cast<int>(getType())
-        << ", transid=0x" << hex << transid_ << dec;
+        << ":" << remote_port_ << ", msg_type=";
+
+    // Try to obtain message type. This may throw if the Message Type option is
+    // not present. Therefore we guard it with try-catch, because we don't want
+    // toText method to throw.
+    try {
+        uint8_t msg_type = getType();
+        output << getName(msg_type) << " (" << static_cast<int>(msg_type) << ")";
+
+    } catch (...) {
+        // Message Type option is missing.
+        output << "(missing)";
+    }
+
+    output << ", transid=0x" << hex << transid_ << dec;
 
     if (!options_.empty()) {
         output << "," << std::endl << "options:";
@@ -371,6 +384,9 @@ Pkt4::toText() const {
              opt != options_.end(); ++opt) {
             output << std::endl << opt->second->toText(2);
         }
+
+    } else {
+        output << ", message contains no options";
     }
 
     return (output.str());

src/lib/dhcp/tests/option_int_array_unittest.cc

@@ -464,13 +464,15 @@ TEST_F(OptionIntArrayTest, addValuesInt32) {
     addValuesTest<int16_t>();
 }
 
+// This test checks that the option is correctly converted into
+// the textual format.
 TEST_F(OptionIntArrayTest, toText) {
     OptionUint32Array option(Option::V4, 128);
     option.addValue(1);
     option.addValue(32);
     option.addValue(324);
 
-    EXPECT_EQ("type=128, len=012: 1 (uint32) 32 (uint32) 324 (uint32)",
+    EXPECT_EQ("type=128, len=012: 1(uint32) 32(uint32) 324(uint32)",
               option.toText());
 }
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -976,13 +976,26 @@ TEST_F(Pkt4Test, toText) {
     pkt.addOption(OptionPtr(new OptionString(Option::V4, 87, "lorem ipsum")));
 
     EXPECT_EQ("local_address=192.0.2.34:67, remote_adress=192.10.33.4:68, "
-              "msg_type=1, transid=0x9ef,\n"
+              "msg_type=DHCPDISCOVER (1), transid=0x9ef,\n"
               "options:\n"
               "  type=053, len=001: 01\n"
               "  type=087, len=011: \"lorem ipsum\" (string)\n"
               "  type=123, len=004: 192.0.2.3\n"
               "  type=156, len=004: 123456 (uint32)",
               pkt.toText());
+
+    // Now remove all options, including Message Type and check if the
+    // information about lack of any options is displayed properly.
+    pkt.delOption(123);
+    pkt.delOption(156);
+    pkt.delOption(87);
+    pkt.delOption(53);
+
+    EXPECT_EQ("local_address=192.0.2.34:67, remote_adress=192.10.33.4:68, "
+              "msg_type=(missing), transid=0x9ef, "
+              "message contains no options",
+              pkt.toText());
+
 }
 
 } // end of anonymous namespace