[3054] Changes as a result of review

The main change is that the list of libraries failing validation is
returned as a vector of strings instead of as a single comma-separated
string.

src/lib/hooks/callout_manager.h

@@ -132,8 +132,7 @@ public:
     ///
     /// @param num_libraries Number of loaded libraries.
     ///
-    /// @throw isc::BadValue if the number of libraries is less than or equal
-    ///        to 0, or if the pointer to the server hooks object is empty.
+    /// @throw isc::BadValue if the number of libraries is less than 0,
     CalloutManager(int num_libraries = 0);
 
     /// @brief Register a callout on a hook for the current library

src/lib/hooks/hooks_manager.cc

@@ -191,7 +191,7 @@ HooksManager::postCalloutsLibraryHandle() {
 
 // Validate libraries
 
-std::string
+std::vector<std::string>
 HooksManager::validateLibraries(const std::vector<std::string>& libraries) {
     return (LibraryManagerCollection::validateLibraries(libraries));
 }

src/lib/hooks/hooks_manager.h

@@ -190,13 +190,9 @@ public:
     ///
     /// @param List of libraries to be validated.
     ///
-    /// @return An empty string if all libraries validated.  Otherwise it is
-    ///         the names of the libraries that failed validation, separated
-    ///         by a command and a space.  The configuration code can return
-    ///         this to bindctl as an indication of the problem. (Note that
-    ///         validation failures are logged, so more information can be
-    ///         obtained if necessary.)
-    static std::string validateLibraries(
+    /// @return An empty vector if all libraries validated.  Otherwise it
+    ///         holds the names of the libraries that failed validation.
+    static std::vector<std::string> validateLibraries(
                        const std::vector<std::string>& libraries);
 
     /// Index numbers for pre-defined hooks.

src/lib/hooks/library_manager_collection.cc

@@ -111,17 +111,14 @@ LibraryManagerCollection::getLoadedLibraryCount() const {
 }
 
 // Validate the libraries.
-std::string
+std::vector<std::string>
 LibraryManagerCollection::validateLibraries(
                           const std::vector<std::string>& libraries) {
 
-    std::string failures("");
+    std::vector<std::string> failures;
     for (int i = 0; i < libraries.size(); ++i) {
         if (!LibraryManager::validateLibrary(libraries[i])) {
-            if (!failures.empty()) {
-                failures += std::string(", ");
-            }
-            failures += libraries[i];
+            failures.push_back(libraries[i]);
         }
     }
 

src/lib/hooks/library_manager_collection.h

@@ -139,9 +139,9 @@ public:
     ///
     /// @param libraries List of libraries to validate
     ///
-    /// @return Comma-separated list of libraries that faled to validate, or
-    ///         the empty string if all validated.
-    static std::string
+    /// @return Vector of libraries that faled to validate, or an empty vector
+    ///         if all validated.
+    static std::vector<std::string>
     validateLibraries(const std::vector<std::string>& libraries);
 
 protected:

src/lib/hooks/tests/hooks_manager_unittest.cc

@@ -450,42 +450,48 @@ TEST_F(HooksManagerTest, LibraryNames) {
 // Test the library validation function.
 
 TEST_F(HooksManagerTest, validateLibraries) {
-    const std::string empty;
-    const std::string separator(", ");
+    // Vector of libraries that failed validation
+    std::vector<std::string> failed;
 
     // Test different vectors of libraries.
 
     // No libraries should return a success.
     std::vector<std::string> libraries;
-    EXPECT_EQ(empty, HooksManager::validateLibraries(libraries));
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Single valid library should validate.
     libraries.clear();
     libraries.push_back(BASIC_CALLOUT_LIBRARY);
-    EXPECT_EQ(empty, HooksManager::validateLibraries(libraries));
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Multiple valid libraries should succeed.
     libraries.clear();
     libraries.push_back(BASIC_CALLOUT_LIBRARY);
     libraries.push_back(FULL_CALLOUT_LIBRARY);
     libraries.push_back(UNLOAD_CALLOUT_LIBRARY);
-    EXPECT_EQ(empty, HooksManager::validateLibraries(libraries));
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Single invalid library should fail.
     libraries.clear();
     libraries.push_back(NOT_PRESENT_LIBRARY);
-    EXPECT_EQ(std::string(NOT_PRESENT_LIBRARY),
-              HooksManager::validateLibraries(libraries));
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed == libraries);
 
     // Multiple invalid libraries should fail.
     libraries.clear();
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
     libraries.push_back(NO_VERSION_LIBRARY);
     libraries.push_back(FRAMEWORK_EXCEPTION_LIBRARY);
-    std::string expected = std::string(INCORRECT_VERSION_LIBRARY) + separator +
-                           std::string(NO_VERSION_LIBRARY) + separator +
-                           std::string(FRAMEWORK_EXCEPTION_LIBRARY);
-    EXPECT_EQ(expected, HooksManager::validateLibraries(libraries));
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed == libraries);
 
     // Combination of valid and invalid (first one valid) should fail.
     libraries.clear();
@@ -493,9 +499,12 @@ TEST_F(HooksManagerTest, validateLibraries) {
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
     libraries.push_back(NO_VERSION_LIBRARY);
 
-    expected = std::string(INCORRECT_VERSION_LIBRARY) + separator +
-               std::string(NO_VERSION_LIBRARY);
-    EXPECT_EQ(expected, HooksManager::validateLibraries(libraries));
+    std::vector<std::string> expected_failures;
+    expected_failures.push_back(INCORRECT_VERSION_LIBRARY);
+    expected_failures.push_back(NO_VERSION_LIBRARY);
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed == expected_failures);
 
     // Combination of valid and invalid (first one invalid) should fail.
     libraries.clear();
@@ -503,9 +512,12 @@ TEST_F(HooksManagerTest, validateLibraries) {
     libraries.push_back(FULL_CALLOUT_LIBRARY);
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
 
-    expected = std::string(NO_VERSION_LIBRARY) + separator +
-               std::string(INCORRECT_VERSION_LIBRARY);
-    EXPECT_EQ(expected, HooksManager::validateLibraries(libraries));
+    expected_failures.clear();
+    expected_failures.push_back(NO_VERSION_LIBRARY);
+    expected_failures.push_back(INCORRECT_VERSION_LIBRARY);
+
+    failed = HooksManager::validateLibraries(libraries);
+    EXPECT_TRUE(failed == expected_failures);
 }
 
 

src/lib/hooks/tests/library_manager_collection_unittest.cc

@@ -178,42 +178,48 @@ TEST_F(LibraryManagerCollectionTest, LibraryNames) {
 // Test the library validation function.
 
 TEST_F(LibraryManagerCollectionTest, validateLibraries) {
-    const std::string empty;
-    const std::string separator(", ");
+    // Vector of libraries that failed validation
+    std::vector<std::string> failed;
 
     // Test different vectors of libraries.
 
     // No libraries should return a success.
     std::vector<std::string> libraries;
-    EXPECT_EQ(empty, LibraryManagerCollection::validateLibraries(libraries));
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Single valid library should validate.
     libraries.clear();
     libraries.push_back(BASIC_CALLOUT_LIBRARY);
-    EXPECT_EQ(empty, LibraryManagerCollection::validateLibraries(libraries));
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Multiple valid libraries should succeed.
     libraries.clear();
     libraries.push_back(BASIC_CALLOUT_LIBRARY);
     libraries.push_back(FULL_CALLOUT_LIBRARY);
     libraries.push_back(UNLOAD_CALLOUT_LIBRARY);
-    EXPECT_EQ(empty, LibraryManagerCollection::validateLibraries(libraries));
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed.empty());
 
     // Single invalid library should fail.
     libraries.clear();
     libraries.push_back(NOT_PRESENT_LIBRARY);
-    EXPECT_EQ(std::string(NOT_PRESENT_LIBRARY),
-              LibraryManagerCollection::validateLibraries(libraries));
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed == libraries);
 
     // Multiple invalid libraries should fail.
     libraries.clear();
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
     libraries.push_back(NO_VERSION_LIBRARY);
     libraries.push_back(FRAMEWORK_EXCEPTION_LIBRARY);
-    std::string expected = std::string(INCORRECT_VERSION_LIBRARY) + separator +
-                           std::string(NO_VERSION_LIBRARY) + separator +
-                           std::string(FRAMEWORK_EXCEPTION_LIBRARY);
-    EXPECT_EQ(expected, LibraryManagerCollection::validateLibraries(libraries));
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed == libraries);
 
     // Combination of valid and invalid (first one valid) should fail.
     libraries.clear();
@@ -221,9 +227,12 @@ TEST_F(LibraryManagerCollectionTest, validateLibraries) {
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
     libraries.push_back(NO_VERSION_LIBRARY);
 
-    expected = std::string(INCORRECT_VERSION_LIBRARY) + separator +
-               std::string(NO_VERSION_LIBRARY);
-    EXPECT_EQ(expected, LibraryManagerCollection::validateLibraries(libraries));
+    std::vector<std::string> expected_failures;
+    expected_failures.push_back(INCORRECT_VERSION_LIBRARY);
+    expected_failures.push_back(NO_VERSION_LIBRARY);
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed == expected_failures);
 
     // Combination of valid and invalid (first one invalid) should fail.
     libraries.clear();
@@ -231,9 +240,12 @@ TEST_F(LibraryManagerCollectionTest, validateLibraries) {
     libraries.push_back(FULL_CALLOUT_LIBRARY);
     libraries.push_back(INCORRECT_VERSION_LIBRARY);
 
-    expected = std::string(NO_VERSION_LIBRARY) + separator +
-               std::string(INCORRECT_VERSION_LIBRARY);
-    EXPECT_EQ(expected, LibraryManagerCollection::validateLibraries(libraries));
+    expected_failures.clear();
+    expected_failures.push_back(NO_VERSION_LIBRARY);
+    expected_failures.push_back(INCORRECT_VERSION_LIBRARY);
+
+    failed = LibraryManagerCollection::validateLibraries(libraries);
+    EXPECT_TRUE(failed == expected_failures);
 }
 
 } // Anonymous namespace