From 9dc99c08dc93bba2e25c55e7e93b88a5fb573744 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 30 Jan 2018 13:26:14 +0100 Subject: [PATCH] [5457] Addressed review comments. --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 3 ++- src/bin/dhcp4/dhcp4_hooks.dox | 6 +++--- src/bin/dhcp4/dhcp4_srv.cc | 7 ++++++- src/bin/dhcp4/tests/callout_library_1.cc | 4 ++-- src/bin/dhcp4/tests/callout_library_2.cc | 4 ++-- src/bin/dhcp4/tests/hooks_unittest.cc | 4 ++-- src/lib/hooks/parking_lots.h | 10 ++++++++++ src/lib/hooks/tests/hooks_manager_unittest.cc | 8 ++++---- src/lib/hooks/tests/parking_lots_unittest.cc | 6 +++--- 9 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index ae0dc5f9bd..3cf0f0bd46 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -39,6 +39,7 @@ struct CtrlDhcp4Hooks { CtrlDhcp4Hooks() { hooks_index_dhcp4_srv_configured_ = HooksManager::registerHook("dhcp4_srv_configured"); } + }; // Declare a Hooks object. As this is outside any function or method, it @@ -653,7 +654,7 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { if (HooksManager::calloutsPresent(Hooks.hooks_index_dhcp4_srv_configured_)) { CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); - callout_handle->setArgument("io_service", srv->getIOService()); + callout_handle->setArgument("io_context", srv->getIOService()); callout_handle->setArgument("json_config", config); callout_handle->setArgument("server_config", CfgMgr::instance().getStagingCfg()); diff --git a/src/bin/dhcp4/dhcp4_hooks.dox b/src/bin/dhcp4/dhcp4_hooks.dox index 0d804f2a91..2ae0289aa0 100644 --- a/src/bin/dhcp4/dhcp4_hooks.dox +++ b/src/bin/dhcp4/dhcp4_hooks.dox @@ -48,15 +48,15 @@ to the end of this list. @subsection dhcp4HooksDhcpv4SrvConfigured dhcp4_srv_configured - @b Arguments: - - name: @b io_service, type: isc::asiolink::IOServicePtr, direction: in + - name: @b io_context, type: isc::asiolink::IOServicePtr, direction: in - name: @b json_config, type: isc::data::ConstElementPtr, direction: in - name: @b server_config, type: isc::dhcp::SrvConfigPtr, direction: in - @b Description: this callout is executed when the server has completed its (re)configuration. The server provides received and parsed configuration - structures to the hook library. It also provides a pointer to the io_service + structures to the hook library. It also provides a pointer to the IOService object which is used by the server to run asynchronous operations. The hooks - libraries can use this IO service object to schedule asynchronous tasks which + libraries can use this IOService object to schedule asynchronous tasks which are triggered by the DHCP server's main loop. The hook library should hold the provided pointer until the library is unloaded. diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 2ca899530f..74ddc3a667 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -78,6 +78,8 @@ using namespace isc::log; using namespace isc::stats; using namespace std; +namespace { + /// Structure that holds registered hook indexes struct Dhcp4Hooks { int hook_index_buffer4_receive_; ///< index for "buffer4_receive" hook point @@ -104,12 +106,15 @@ struct Dhcp4Hooks { } }; +} // end of anonymous namespace + // Declare a Hooks object. As this is outside any function or method, it // will be instantiated (and the constructor run) when the module is loaded. // As a result, the hook indexes will be defined before any method in this // module is called. Dhcp4Hooks Hooks; + namespace isc { namespace dhcp { @@ -1062,7 +1067,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) { } callout_handle->setArgument("leases4", new_leases); - Lease4CollectionPtr deleted_leases(new Lease4Collection); + Lease4CollectionPtr deleted_leases(new Lease4Collection()); if (ctx->old_lease_) { if ((!ctx->new_lease_) || (ctx->new_lease_->addr_ != ctx->old_lease_->addr_)) { deleted_leases->push_back(ctx->old_lease_); diff --git a/src/bin/dhcp4/tests/callout_library_1.cc b/src/bin/dhcp4/tests/callout_library_1.cc index 72045f28d0..af66942ae9 100644 --- a/src/bin/dhcp4/tests/callout_library_1.cc +++ b/src/bin/dhcp4/tests/callout_library_1.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -11,4 +11,4 @@ /// tests. See callout_common.cc for details. static const int LIBRARY_NUMBER = 1; -#include "callout_library_common.h" +#include diff --git a/src/bin/dhcp4/tests/callout_library_2.cc b/src/bin/dhcp4/tests/callout_library_2.cc index 3c7bbe00ff..8135e2b1cd 100644 --- a/src/bin/dhcp4/tests/callout_library_2.cc +++ b/src/bin/dhcp4/tests/callout_library_2.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -11,4 +11,4 @@ /// tests. See callout_common.cc for details. static const int LIBRARY_NUMBER = 2; -#include "callout_library_common.h" +#include diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc index 2bf7428e4e..b43d813425 100644 --- a/src/bin/dhcp4/tests/hooks_unittest.cc +++ b/src/bin/dhcp4/tests/hooks_unittest.cc @@ -2688,7 +2688,7 @@ TEST_F(LoadUnloadDhcpv4SrvTest, Dhcpv4SrvConfigured) { // The dhcp4_srv_configured should have been invoked and the provided // parameters should be recorded. EXPECT_TRUE(checkMarkerFile(SRV_CONFIG_MARKER_FILE, - "3io_servicejson_configserver_config")); + "3io_contextjson_configserver_config")); // Destroy the server, instance which should unload the libraries. srv.reset(); @@ -2698,5 +2698,5 @@ TEST_F(LoadUnloadDhcpv4SrvTest, Dhcpv4SrvConfigured) { EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "3")); EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, "3")); EXPECT_TRUE(checkMarkerFile(SRV_CONFIG_MARKER_FILE, - "3io_servicejson_configserver_config")); + "3io_contextjson_configserver_config")); } diff --git a/src/lib/hooks/parking_lots.h b/src/lib/hooks/parking_lots.h index 991fe3d323..7ee9ec0eae 100644 --- a/src/lib/hooks/parking_lots.h +++ b/src/lib/hooks/parking_lots.h @@ -52,6 +52,11 @@ namespace hooks { /// be unparked, but the object will be unparked when all callouts call this /// function, i.e. when all callouts signal completion of their respective /// asynchronous operations. +/// +/// The types of the parked objects provided as T parameter of respective +/// functions are most often shared pointers. One should not use references +/// to parked objects nor references to shared pointers to avoid premature +/// destruction of the parked objects. class ParkingLot { public: @@ -218,6 +223,11 @@ typedef boost::shared_ptr ParkingLotPtr; /// The handle is provided to the callouts which can reference and unpark /// parked objects. The callouts should not park objects, therefore this /// operation is not available. +/// +/// The types of the parked objects provided as T parameter of respective +/// functions are most often shared pointers. One should not use references +/// to parked objects nor references to shared pointers to avoid premature +/// destruction of the parked objects. class ParkingLotHandle { public: diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index ff985a22e7..089c894f4a 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -787,7 +787,7 @@ TEST_F(HooksManagerTest, Parking) { // got unparked. ASSERT_NO_THROW( HooksManager::park("hookpt_one", "foo", - [this, &unparked] { + [&unparked] { unparked = true; }) ); @@ -848,7 +848,7 @@ TEST_F(HooksManagerTest, ServerUnpark) { // can later test the value of this flag to verify when exactly the packet // got unparked. HooksManager::park("hookpt_one", "foo", - [this, &unparked] { + [&unparked] { unparked = true; }); @@ -890,7 +890,7 @@ TEST_F(HooksManagerTest, ServerDropParked) { // can later test the value of this flag to verify when exactly the packet // got unparked. HooksManager::park("hookpt_one", "foo", - [this, &unparked] { + [&unparked] { unparked = true; }); @@ -938,7 +938,7 @@ TEST_F(HooksManagerTest, UnloadBeforeUnpark) { // can later test the value of this flag to verify when exactly the packet // got unparked. HooksManager::park("hookpt_one", "foo", - [this, &unparked] { + [&unparked] { unparked = true; }); diff --git a/src/lib/hooks/tests/parking_lots_unittest.cc b/src/lib/hooks/tests/parking_lots_unittest.cc index fe7a2039db..99b62ca83f 100644 --- a/src/lib/hooks/tests/parking_lots_unittest.cc +++ b/src/lib/hooks/tests/parking_lots_unittest.cc @@ -43,7 +43,7 @@ TEST(ParkingLotsTest, createGetParkingLot) { TEST(ParkingLotTest, parkWithoutReferencing) { ParkingLot parking_lot; std::string parked_object = "foo"; - EXPECT_THROW(parking_lot.park(parked_object, [this] { + EXPECT_THROW(parking_lot.park(parked_object, [] { }), InvalidOperation); } @@ -62,7 +62,7 @@ TEST(ParkingLotTest, unpark) { // This flag will indicate if the callback has been called. bool unparked = false; - ASSERT_NO_THROW(parking_lot->park(parked_object, [this, &unparked] { + ASSERT_NO_THROW(parking_lot->park(parked_object, [&unparked] { unparked = true; })); @@ -96,7 +96,7 @@ TEST(ParkingLotTest, drop) { // This flag will indicate if the callback has been called. bool unparked = false; - ASSERT_NO_THROW(parking_lot->park(parked_object, [this, &unparked] { + ASSERT_NO_THROW(parking_lot->park(parked_object, [&unparked] { unparked = true; }));