[2681] Address review comments

src/bin/dhcp4/dhcp4_messages.mes

@@ -1,4 +1,4 @@
-# Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2012-2013  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above
@@ -94,7 +94,7 @@ server is about to open sockets on the specified port.
 The IPv4 DHCP server has received a packet that it is unable to
 interpret. The reason why the packet is invalid is included in the message.
 
-% DHCP4_PACKET_PROCESS_FAIL failed to process received packet: %1
+% DHCP4_PACKET_PROCESS_FAIL failed to process packet received from %1: %2
 This is a general catch-all message indicating that the processing of a
 received packet failed.  The reason is given in the message.  The server
 will not send a response but will instead ignore the packet.

src/bin/dhcp4/dhcp4_srv.cc

@@ -173,8 +173,16 @@ Dhcpv4Srv::run() {
                 // as a debug message because debug is disabled by default -
                 // it prevents a DDOS attack based on the sending of problem
                 // packets.)
-                LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC,
-                          DHCP4_PACKET_PROCESS_FAIL).arg(e.what());
+                if (dhcp4_logger.isDebugEnabled(DBG_DHCP4_BASIC)) {
+                    std::string source = "unknown";
+                    HWAddrPtr hwptr = query->getHWAddr();
+                    if (hwptr) {
+                        source = hwptr->toText();
+                    }
+                    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC,
+                              DHCP4_PACKET_PROCESS_FAIL)
+                              .arg(source).arg(e.what());
+                }
             }
 
             if (rsp) {

src/bin/dhcp6/dhcp6_messages.mes

@@ -1,4 +1,4 @@
-# Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2012-2013  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above

src/lib/dhcpsrv/alloc_engine.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -170,62 +170,62 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
                               const IOAddress& hint,
                               bool fake_allocation /* = false */ ) {
 
-    // That check is not necessary. We create allocator in AllocEngine
-    // constructor
-    if (!allocator_) {
-        isc_throw(InvalidOperation, "No allocator selected");
-    }
+    try {
+        // That check is not necessary. We create allocator in AllocEngine
+        // constructor
+        if (!allocator_) {
+            isc_throw(InvalidOperation, "No allocator selected");
+        }
 
-    // check if there's existing lease for that subnet/duid/iaid combination.
-    Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(*duid, iaid, subnet->getID());
-    if (existing) {
-        // we have a lease already. This is a returning client, probably after
-        // his reboot.
-        return (existing);
-    }
+        // check if there's existing lease for that subnet/duid/iaid combination.
+        Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(*duid, iaid, subnet->getID());
+        if (existing) {
+            // we have a lease already. This is a returning client, probably after
+            // his reboot.
+            return (existing);
+        }
 
-    // check if the hint is in pool and is available
-    if (subnet->inPool(hint)) {
-        existing = LeaseMgrFactory::instance().getLease6(hint);
-        if (!existing) {
-            /// @todo: check if the hint is reserved once we have host support
-            /// implemented
+        // check if the hint is in pool and is available
+        if (subnet->inPool(hint)) {
+            existing = LeaseMgrFactory::instance().getLease6(hint);
+            if (!existing) {
+                /// @todo: check if the hint is reserved once we have host support
+                /// implemented
 
-            // the hint is valid and not currently used, let's create a lease for it
-            Lease6Ptr lease = createLease6(subnet, duid, iaid, hint, fake_allocation);
+                // the hint is valid and not currently used, let's create a lease for it
+                Lease6Ptr lease = createLease6(subnet, duid, iaid, hint, fake_allocation);
 
-            // It can happen that the lease allocation failed (we could have lost
-            // the race condition. That means that the hint is lo longer usable and
-            // we need to continue the regular allocation path.
-            if (lease) {
-                return (lease);
-            }
-        } else {
-            if (existing->expired()) {
-                return (reuseExpiredLease(existing, subnet, duid, iaid,
-                                          fake_allocation));
-            }
+                // It can happen that the lease allocation failed (we could have lost
+                // the race condition. That means that the hint is lo longer usable and
+                // we need to continue the regular allocation path.
+                if (lease) {
+                    return (lease);
+                }
+            } else {
+                if (existing->expired()) {
+                    return (reuseExpiredLease(existing, subnet, duid, iaid,
+                                              fake_allocation));
+                }
 
+            }
         }
-    }
 
-    // Hint is in the pool but is not available. Search the pool until first of
-    // the following occurs:
-    // - we find a free address
-    // - we find an address for which the lease has expired
-    // - we exhaust number of tries
-    //
-    // @todo: Current code does not handle pool exhaustion well. It will be
-    // improved. Current problems:
-    // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
-    // 10 addresses), we will iterate over it 100 times before giving up
-    // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
-    // 3. the whole concept of infinite attempts is just asking for infinite loop
-    // We may consider some form or reference counting (this pool has X addresses
-    // left), but this has one major problem. We exactly control allocation
-    // moment, but we currently do not control expiration time at all
+        // Hint is in the pool but is not available. Search the pool until first of
+        // the following occurs:
+        // - we find a free address
+        // - we find an address for which the lease has expired
+        // - we exhaust number of tries
+        //
+        // @todo: Current code does not handle pool exhaustion well. It will be
+        // improved. Current problems:
+        // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
+        // 10 addresses), we will iterate over it 100 times before giving up
+        // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
+        // 3. the whole concept of infinite attempts is just asking for infinite loop
+        // We may consider some form or reference counting (this pool has X addresses
+        // left), but this has one major problem. We exactly control allocation
+        // moment, but we currently do not control expiration time at all
 
-    try {
         unsigned int i = attempts_;
         do {
             IOAddress candidate = allocator_->pickAddress(subnet, duid, hint);
@@ -277,82 +277,82 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
                               const IOAddress& hint,
                               bool fake_allocation /* = false */ ) {
 
-    // Allocator is always created in AllocEngine constructor and there is
-    // currently no other way to set it, so that check is not really necessary.
-    if (!allocator_) {
-        isc_throw(InvalidOperation, "No allocator selected");
-    }
-
-    // Check if there's existing lease for that subnet/clientid/hwaddr combination.
-    Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID());
-    if (existing) {
-        // We have a lease already. This is a returning client, probably after
-        // its reboot.
-        existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
-        if (existing) {
-            return (existing);
+    try {
+        // Allocator is always created in AllocEngine constructor and there is
+        // currently no other way to set it, so that check is not really necessary.
+        if (!allocator_) {
+            isc_throw(InvalidOperation, "No allocator selected");
         }
 
-        // If renewal failed (e.g. the lease no longer matches current configuration)
-        // let's continue the allocation process
-    }
-
-    if (clientid) {
-        existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID());
+        // Check if there's existing lease for that subnet/clientid/hwaddr combination.
+        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID());
         if (existing) {
-            // we have a lease already. This is a returning client, probably after
+            // We have a lease already. This is a returning client, probably after
             // its reboot.
             existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
-            // @todo: produce a warning. We haven't found him using MAC address, but
-            // we found him using client-id
             if (existing) {
                 return (existing);
             }
+
+            // If renewal failed (e.g. the lease no longer matches current configuration)
+            // let's continue the allocation process
         }
-    }
 
-    // check if the hint is in pool and is available
-    if (subnet->inPool(hint)) {
-        existing = LeaseMgrFactory::instance().getLease4(hint);
-        if (!existing) {
-            /// @todo: Check if the hint is reserved once we have host support
-            /// implemented
+        if (clientid) {
+            existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID());
+            if (existing) {
+                // we have a lease already. This is a returning client, probably after
+                // its reboot.
+                existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
+                // @todo: produce a warning. We haven't found him using MAC address, but
+                // we found him using client-id
+                if (existing) {
+                    return (existing);
+                }
+            }
+        }
 
-            // The hint is valid and not currently used, let's create a lease for it
-            Lease4Ptr lease = createLease4(subnet, clientid, hwaddr, hint, fake_allocation);
+        // check if the hint is in pool and is available
+        if (subnet->inPool(hint)) {
+            existing = LeaseMgrFactory::instance().getLease4(hint);
+            if (!existing) {
+                /// @todo: Check if the hint is reserved once we have host support
+                /// implemented
 
-            // It can happen that the lease allocation failed (we could have lost
-            // the race condition. That means that the hint is lo longer usable and
-            // we need to continue the regular allocation path.
-            if (lease) {
-                return (lease);
-            }
-        } else {
-            if (existing->expired()) {
-                return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
-                                          fake_allocation));
-            }
+                // The hint is valid and not currently used, let's create a lease for it
+                Lease4Ptr lease = createLease4(subnet, clientid, hwaddr, hint, fake_allocation);
+
+                // It can happen that the lease allocation failed (we could have lost
+                // the race condition. That means that the hint is lo longer usable and
+                // we need to continue the regular allocation path.
+                if (lease) {
+                    return (lease);
+                }
+            } else {
+                if (existing->expired()) {
+                    return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
+                                              fake_allocation));
+                }
 
+            }
         }
-    }
 
-    // Hint is in the pool but is not available. Search the pool until first of
-    // the following occurs:
-    // - we find a free address
-    // - we find an address for which the lease has expired
-    // - we exhaust the number of tries
-    //
-    // @todo: Current code does not handle pool exhaustion well. It will be
-    // improved. Current problems:
-    // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
-    // 10 addresses), we will iterate over it 100 times before giving up
-    // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
-    // 3. the whole concept of infinite attempts is just asking for infinite loop
-    // We may consider some form or reference counting (this pool has X addresses
-    // left), but this has one major problem. We exactly control allocation
-    // moment, but we currently do not control expiration time at all
+        // Hint is in the pool but is not available. Search the pool until first of
+        // the following occurs:
+        // - we find a free address
+        // - we find an address for which the lease has expired
+        // - we exhaust the number of tries
+        //
+        // @todo: Current code does not handle pool exhaustion well. It will be
+        // improved. Current problems:
+        // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
+        // 10 addresses), we will iterate over it 100 times before giving up
+        // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
+        // 3. the whole concept of infinite attempts is just asking for infinite loop
+        // We may consider some form or reference counting (this pool has X addresses
+        // left), but this has one major problem. We exactly control allocation
+        // moment, but we currently do not control expiration time at all
 
-    try {
         unsigned int i = attempts_;
         do {
             IOAddress candidate = allocator_->pickAddress(subnet, clientid, hint);