From: Razvan Becheriu Date: Fri, 7 Feb 2020 11:06:57 +0000 (+0200) Subject: [#957] addressed review X-Git-Tag: Kea-1.7.5~69 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc83b6ceffdbdf4a4677accaaa70fa63394141b2;p=thirdparty%2Fkea.git [#957] addressed review --- diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc index 08ef556bd5..7fd83ee0f3 100644 --- a/src/lib/hooks/callout_handle.cc +++ b/src/lib/hooks/callout_handle.cc @@ -23,10 +23,9 @@ namespace hooks { // Constructor. CalloutHandle::CalloutHandle(const boost::shared_ptr& manager, const boost::shared_ptr& lmcoll) - : current_library_(-1), current_hook_(-1), lm_collection_(lmcoll), - arguments_(), context_collection_(), manager_(manager), - server_hooks_(ServerHooks::getServerHooks()), - next_step_(NEXT_STEP_CONTINUE) { + : lm_collection_(lmcoll), arguments_(), context_collection_(), + manager_(manager), server_hooks_(ServerHooks::getServerHooks()), + current_library_(-1), current_hook_(-1), next_step_(NEXT_STEP_CONTINUE) { // Call the "context_create" hook. We should be OK doing this - although // the constructor has not finished running, all the member variables diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h index 42adfed136..164c4acf76 100644 --- a/src/lib/hooks/callout_handle.h +++ b/src/lib/hooks/callout_handle.h @@ -48,7 +48,6 @@ public: // Forward declaration of the library handle and related collection classes. class CalloutManager; -class LibraryHandle; class LibraryManagerCollection; /// @brief Per-packet callout handle @@ -74,12 +73,6 @@ class LibraryManagerCollection; /// is created in the "context_create" callout and destroyed in the /// "context_destroy" callout. The information is accessed through the /// {get,set}Context() methods. -/// -/// - Per-library handle (LibraryHandle). The library handle allows the -/// callout to dynamically register and deregister callouts. In the latter -/// case, only functions registered by functions in the same library as the -/// callout doing the deregistration can be removed: callouts registered by -/// other libraries cannot be modified. class CalloutHandle { public: @@ -334,19 +327,33 @@ public: /// @return pointer to the parking lot handle ParkingLotHandlePtr getParkingLotHandlePtr() const; - /// @brief Current library. + /// @brief Get current library index /// - /// When a call is made to @ref CalloutManager::callCallouts, this holds - /// the index of the current library. It is set to an invalid value (-1) - /// otherwise. - int current_library_; + /// @return The current library index + int getCurrentLibrary() const { + return (current_library_); + } - /// @brief Current hook. + /// @brief Set cureent library index /// - /// When a call is made to @ref CalloutManager::callCallouts, this holds - /// the index of the current hook. It is set to an invalid value (-1) - /// otherwise. - int current_hook_; + /// @param library_index The library index + void setCurrentLibrary(int library_index) { + current_library_ = library_index; + } + + /// @brief Get current hook index + /// + /// @return The current hook index + int getCurrentHook() const { + return (current_hook_); + } + + /// @brief Set cureent hook index + /// + /// @param hook_index The hook index + void setCurrentHook(int hook_index) { + current_hook_ = hook_index; + } private: @@ -407,6 +414,20 @@ private: /// a reference instead of accessing the singleton within the code. ServerHooks& server_hooks_; + /// @brief Current library. + /// + /// When a call is made to @ref CalloutManager::callCallouts, this holds + /// the index of the current library. It is set to an invalid value (-1) + /// otherwise. + int current_library_; + + /// @brief Current hook. + /// + /// When a call is made to @ref CalloutManager::callCallouts, this holds + /// the index of the current hook. It is set to an invalid value (-1) + /// otherwise. + int current_hook_; + /// Next processing step, indicating what the server should do next. CalloutNextStep next_step_; }; diff --git a/src/lib/hooks/callout_manager.cc b/src/lib/hooks/callout_manager.cc index b61670b21e..afed2df60e 100644 --- a/src/lib/hooks/callout_manager.cc +++ b/src/lib/hooks/callout_manager.cc @@ -143,14 +143,7 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { // Set the current hook index. This is used should a callout wish to // determine to what hook it is attached. - callout_handle.current_hook_ = hook_index; - - // Duplicate the callout vector for this hook and work through that. - // This step is needed because we allow dynamic registration and - // deregistration of callouts. If a callout attached to a hook modified - // the list of callouts on that hook, the underlying CalloutVector would - // change and potentially affect the iteration through that vector. - CalloutVector callouts(hook_vector_[hook_index]); + callout_handle.setCurrentHook(hook_index); // This object will be used to measure execution time of each callout // and the total time spent in callouts for this hook point. @@ -158,15 +151,16 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { // Mark that the callouts begin for the hook. LOG_DEBUG(callouts_logger, HOOKS_DBG_CALLS, HOOKS_CALLOUTS_BEGIN) - .arg(server_hooks_.getName(callout_handle.current_hook_)); + .arg(server_hooks_.getName(callout_handle.getCurrentHook())); // Call all the callouts. - for (CalloutVector::const_iterator i = callouts.begin(); - i != callouts.end(); ++i) { - // In case the callout tries to register or deregister a callout, - // set the current library index to the index associated with the - // library that registered the callout being called. - callout_handle.current_library_ = i->first; + for (CalloutVector::const_iterator i = hook_vector_[hook_index].begin(); + i != hook_vector_[hook_index].end(); ++i) { + // In case the callout requires access to the context associated + // with the library, set the current library index to the index + // associated with the library that registered the callout being + // called. + callout_handle.setCurrentLibrary(i->first); // Call the callout try { @@ -176,14 +170,14 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { if (status == 0) { LOG_DEBUG(callouts_logger, HOOKS_DBG_EXTENDED_CALLS, HOOKS_CALLOUT_CALLED) - .arg(callout_handle.current_library_) - .arg(server_hooks_.getName(callout_handle.current_hook_)) + .arg(callout_handle.getCurrentLibrary()) + .arg(server_hooks_.getName(callout_handle.getCurrentHook())) .arg(PointerConverter(i->second).dlsymPtr()) .arg(stopwatch.logFormatLastDuration()); } else { LOG_ERROR(callouts_logger, HOOKS_CALLOUT_ERROR) - .arg(callout_handle.current_library_) - .arg(server_hooks_.getName(callout_handle.current_hook_)) + .arg(callout_handle.getCurrentLibrary()) + .arg(server_hooks_.getName(callout_handle.getCurrentHook())) .arg(PointerConverter(i->second).dlsymPtr()) .arg(stopwatch.logFormatLastDuration()); } @@ -193,8 +187,8 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { stopwatch.stop(); // Any exception, not just ones based on isc::Exception LOG_ERROR(callouts_logger, HOOKS_CALLOUT_EXCEPTION) - .arg(current_library_) - .arg(server_hooks_.getName(callout_handle.current_hook_)) + .arg(callout_handle.getCurrentLibrary()) + .arg(server_hooks_.getName(callout_handle.getCurrentHook())) .arg(PointerConverter(i->second).dlsymPtr()) .arg(e.what()) .arg(stopwatch.logFormatLastDuration()); @@ -205,13 +199,13 @@ CalloutManager::callCallouts(int hook_index, CalloutHandle& callout_handle) { // Mark end of callout execution. Include the total execution // time for callouts. LOG_DEBUG(callouts_logger, HOOKS_DBG_CALLS, HOOKS_CALLOUTS_COMPLETE) - .arg(server_hooks_.getName(callout_handle.current_hook_)) + .arg(server_hooks_.getName(callout_handle.getCurrentHook())) .arg(stopwatch.logFormatTotalDuration()); // Reset the current hook and library indexes to an invalid value to // catch any programming errors. - callout_handle.current_hook_ = -1; - callout_handle.current_library_ = -1; + callout_handle.setCurrentHook(-1); + callout_handle.setCurrentLibrary(-1); } } diff --git a/src/lib/hooks/callout_manager.h b/src/lib/hooks/callout_manager.h index aba805644f..b6320d58d3 100644 --- a/src/lib/hooks/callout_manager.h +++ b/src/lib/hooks/callout_manager.h @@ -39,22 +39,18 @@ public: /// In operation, the class needs to know two items of data: /// /// - The list of server hooks, which is used in two ways. Firstly, when a -/// callout registers or deregisters a hook, it does so by name: the +/// library registers or deregisters a hook, it does so by name: the /// @ref isc::hooks::ServerHooks object supplies the names of registered /// hooks. Secondly, when the callouts associated with a hook are called by /// the server, the server supplies the index of the relevant hook: this is -/// validated by reference to the list of hook. +/// validated by reference to the list of hooks. /// /// - The number of loaded libraries. Each callout registered by a user /// library is associated with that library, the callout manager storing both /// a pointer to the callout and the index of the library in the list of -/// loaded libraries. Callouts are allowed to dynamically register and -/// deregister callouts in the same library (including themselves): they -/// cannot affect callouts registered by another library. When calling a -/// callout, the callout manager maintains the idea of a "current library -/// index": if the callout calls one of the callout registration functions -/// (indirectly via the @ref LibraryHandle object), the registration -/// functions use the "current library index" in their processing. +/// loaded libraries. When calling a callout, the callout manager maintains +/// the idea of a "current library index": this is used to access the context +/// associated with the library. /// /// These two items of data are supplied when an object of this class is /// constructed. The latter (number of libraries) can be updated after the @@ -72,9 +68,7 @@ public: /// A1 and A2 (in that order) B registers B1 and B2 (in that order) and C /// registers C1 and C2 (in that order). Internally, the callouts are stored /// in the order A1, A2, B1, B2, C1, and C2: this is also the order in which -/// the are called. If B now registers another callout (B3), it is added to -/// the vector after the list of callouts associated with B: the new order is -/// therefore A1, A2, B1, B2, B3, C1 and C2. +/// they are called. /// /// Indexes range between 1 and n (where n is the number of the libraries /// loaded) and are assigned to libraries based on the order the libraries @@ -159,10 +153,9 @@ public: /// @brief Register a callout on a hook for the current library /// - /// Registers a callout function for the current library with a given hook - /// (the index of the "current library" being given by the current_library_ - /// member). The callout is added to the end of the callouts for this - /// library that are associated with that hook. + /// Registers a callout function for the current library with a given hook. + /// The callout is added to the end of the callouts for this library that + /// are associated with that hook. /// /// @param name Name of the hook to which the callout is added. /// @param callout Pointer to the callout function to be registered. @@ -178,8 +171,7 @@ public: /// @brief De-Register a callout on a hook for the current library /// /// Searches through the functions registered by the the current library - /// (the index of the "current library" being given by the current_library_ - /// member) with the named hook and removes all entries matching the + /// with the named hook and removes all entries matching the /// callout. /// /// @param name Name of the hook from which the callout is removed. @@ -198,8 +190,7 @@ public: /// @brief Removes all callouts on a hook for the current library /// /// Removes all callouts associated with a given hook that were registered - /// by the current library (the index of the "current library" being given - /// by the current_library_ member). + /// by the current library. /// /// @param name Name of the hook from which the callouts are removed. /// @param library_index Library index used for registering the callout. @@ -380,7 +371,7 @@ private: /// from global static objects. ServerHooks object creates hooks_ collection /// and CalloutManager creates its own hook_vector_ and both are initialized /// to the same size. All works well so far. However, if some code at a - /// later time (e.g. a hook library) register new hook point, then + /// later time (e.g. a hook library) registers new hook point, then /// ServerHooks::registerHook() will extend its hooks_ collection, but /// the CalloutManager will keep the old hook_vector_ that is too small by /// one. Now when the library is unloaded, deregisterAllCallouts will @@ -441,7 +432,7 @@ private: int num_libraries_; }; -} // namespace util -} // namespace isc +} // namespace util +} // namespace isc #endif // CALLOUT_MANAGER_H diff --git a/src/lib/hooks/tests/handles_unittest.cc b/src/lib/hooks/tests/handles_unittest.cc index 36fba879fe..aab790056e 100644 --- a/src/lib/hooks/tests/handles_unittest.cc +++ b/src/lib/hooks/tests/handles_unittest.cc @@ -536,18 +536,6 @@ TEST_F(HandlesTest, ConstructionDestructionCallouts) { EXPECT_EQ((110 + 120), resultCalloutInt(0)); } -// Dynamic callout registration and deregistration. -// The following are the dynamic registration/deregistration callouts. - - -// Add callout_78_alpha - adds a callout to hook alpha that appends "78x" -// (where "x" is the callout handle) to the current output. - -int -callout78(CalloutHandle& callout_handle) { - return (execute(callout_handle, 7, 8)); -} - // Testing the operation of the "skip" flag. Callouts print the value // they see in the flag and either leave it unchanged, set it or clear it. int @@ -610,7 +598,7 @@ TEST_F(HandlesTest, ReturnSkipSet) { CalloutHandle callout_handle(getCalloutManager()); getCalloutManager()->callCallouts(alpha_index_, callout_handle); - // Check result. For each of visual checking, the expected string is + // Check result. For ease of visual checking, the expected string is // divided into sections corresponding to the blocks of callouts above. EXPECT_EQ(std::string("NNYY" "NNYYN" "NNYN"), common_string_); @@ -639,7 +627,7 @@ TEST_F(HandlesTest, ReturnSkipClear) { CalloutHandle callout_handle(getCalloutManager()); getCalloutManager()->callCallouts(alpha_index_, callout_handle); - // Check result. For each of visual checking, the expected string is + // Check result. For ease of visual checking, the expected string is // divided into sections corresponding to the blocks of callouts above. EXPECT_EQ(std::string("NYY" "NNYNYN" "NNNY"), common_string_);