[2861] Use structure for the command

Replace std::pair with our own structure and add the callback there too.
Update code to match the change. No functional change.

src/bin/auth/datasrc_clients_mgr.h

@@ -82,12 +82,22 @@ enum CommandID {
 typedef boost::function<void ()> FinishedCallback;
 
 /// \brief The data type passed from DataSrcClientsMgr to
-/// DataSrcClientsBuilder.
+///     DataSrcClientsBuilder.
 ///
-/// The first element of the pair is the command ID, and the second element
-/// is its argument.  If the command doesn't take an argument it should be
-/// a null pointer.
-typedef std::pair<CommandID, data::ConstElementPtr> Command;
+/// This just holds the data items together, no logic or protection
+/// is present here.
+struct Command {
+    Command(CommandID id, const data::ConstElementPtr& params,
+            const FinishedCallback& callback) :
+        id(id),
+        params(params),
+        callback(callback)
+    {}
+    CommandID id;
+    data::ConstElementPtr params;
+    FinishedCallback callback;
+};
+
 } // namespace datasrc_clientmgr_internal
 
 /// \brief Frontend to the manager object for data source clients.
@@ -336,13 +346,12 @@ private:
                      const datasrc_clientmgr_internal::FinishedCallback&
                      callback = datasrc_clientmgr_internal::FinishedCallback())
     {
-        (void) callback; // Temporary, remove!
         // The lock will be held until the end of this method.  Only
         // push_back has to be protected, but we can avoid having an extra
         // block this way.
         typename MutexType::Locker locker(queue_mutex_);
         command_queue_.push_back(
-            datasrc_clientmgr_internal::Command(command, arg));
+            datasrc_clientmgr_internal::Command(command, arg, callback));
         cond_.signal();
     }
 
@@ -533,7 +542,7 @@ bool
 DataSrcClientsBuilderBase<MutexType, CondVarType>::handleCommand(
     const Command& command)
 {
-    const CommandID cid = command.first;
+    const CommandID cid = command.id;
     if (cid >= NUM_COMMANDS) {
         // This shouldn't happen except for a bug within this file.
         isc_throw(Unexpected, "internal bug: invalid command, ID: " << cid);
@@ -544,12 +553,12 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::handleCommand(
     };
     LOG_DEBUG(auth_logger, DBGLVL_TRACE_BASIC,
               AUTH_DATASRC_CLIENTS_BUILDER_COMMAND).arg(command_desc.at(cid));
-    switch (command.first) {
+    switch (command.id) {
     case RECONFIGURE:
-        doReconfigure(command.second);
+        doReconfigure(command.params);
         break;
     case LOADZONE:
-        doLoadZone(command.second);
+        doLoadZone(command.params);
         break;
     case SHUTDOWN:
         return (false);

src/bin/auth/tests/datasrc_clients_builder_unittest.cc

@@ -56,8 +56,8 @@ protected:
                     boost::shared_ptr<ConfigurableClientList> >),
         builder(&command_queue, &cond, &queue_mutex, &clients_map, &map_mutex),
         cond(command_queue, delayed_command_queue), rrclass(RRClass::IN()),
-        shutdown_cmd(SHUTDOWN, ConstElementPtr()),
-        noop_cmd(NOOP, ConstElementPtr())
+        shutdown_cmd(SHUTDOWN, ConstElementPtr(), FinishedCallback()),
+        noop_cmd(NOOP, ConstElementPtr(), FinishedCallback())
     {}
 
     void configureZones();      // used for loadzone related tests
@@ -138,7 +138,7 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
     // the error handling
 
     // A command structure we'll modify to send different commands
-    Command reconfig_cmd(RECONFIGURE, ConstElementPtr());
+    Command reconfig_cmd(RECONFIGURE, ConstElementPtr(), FinishedCallback());
 
     // Initially, no clients should be there
     EXPECT_TRUE(clients_map->empty());
@@ -166,7 +166,7 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
         "}"
     );
 
-    reconfig_cmd.second = good_config;
+    reconfig_cmd.params = good_config;
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_EQ(1, clients_map->size());
     EXPECT_EQ(1, map_mutex.lock_count);
@@ -177,7 +177,7 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
     // If a 'bad' command argument got here, the config validation should
     // have failed already, but still, the handler should return true,
     // and the clients_map should not be updated.
-    reconfig_cmd.second = Element::create("{ \"foo\": \"bar\" }");
+    reconfig_cmd.params = Element::create("{ \"foo\": \"bar\" }");
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_EQ(working_config_clients, clients_map);
     // Building failed, so map mutex should not have been locked again
@@ -185,7 +185,7 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
 
     // The same for a configuration that has bad data for the type it
     // specifies
-    reconfig_cmd.second = bad_config;
+    reconfig_cmd.params = bad_config;
     builder.handleCommand(reconfig_cmd);
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_EQ(working_config_clients, clients_map);
@@ -194,21 +194,21 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
 
     // The same goes for an empty parameter (it should at least be
     // an empty map)
-    reconfig_cmd.second = ConstElementPtr();
+    reconfig_cmd.params = ConstElementPtr();
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_EQ(working_config_clients, clients_map);
     EXPECT_EQ(1, map_mutex.lock_count);
 
     // Reconfigure again with the same good clients, the result should
     // be a different map than the original, but not an empty one.
-    reconfig_cmd.second = good_config;
+    reconfig_cmd.params = good_config;
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_NE(working_config_clients, clients_map);
     EXPECT_EQ(1, clients_map->size());
     EXPECT_EQ(2, map_mutex.lock_count);
 
     // And finally, try an empty config to disable all datasource clients
-    reconfig_cmd.second = Element::createMap();
+    reconfig_cmd.params = Element::createMap();
     EXPECT_TRUE(builder.handleCommand(reconfig_cmd));
     EXPECT_EQ(0, clients_map->size());
     EXPECT_EQ(3, map_mutex.lock_count);
@@ -224,7 +224,8 @@ TEST_F(DataSrcClientsBuilderTest, shutdown) {
 TEST_F(DataSrcClientsBuilderTest, badCommand) {
     // out-of-range command ID
     EXPECT_THROW(builder.handleCommand(Command(NUM_COMMANDS,
-                                               ConstElementPtr())),
+                                               ConstElementPtr(),
+                                               FinishedCallback())),
                  isc::Unexpected);
 }
 
@@ -308,7 +309,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZone) {
 
     const Command loadzone_cmd(LOADZONE, Element::fromJSON(
                                    "{\"class\": \"IN\","
-                                   " \"origin\": \"test1.example\"}"));
+                                   " \"origin\": \"test1.example\"}"),
+                               FinishedCallback());
     EXPECT_TRUE(builder.handleCommand(loadzone_cmd));
 
     // loadZone involves two critical sections: one for getting the zone
@@ -369,7 +371,8 @@ TEST_F(DataSrcClientsBuilderTest,
     // Now send the command to reload it
     const Command loadzone_cmd(LOADZONE, Element::fromJSON(
                                    "{\"class\": \"IN\","
-                                   " \"origin\": \"example.org\"}"));
+                                   " \"origin\": \"example.org\"}"),
+                               FinishedCallback());
     EXPECT_TRUE(builder.handleCommand(loadzone_cmd));
     // And now it should be present too.
     EXPECT_EQ(ZoneFinder::SUCCESS,
@@ -380,7 +383,8 @@ TEST_F(DataSrcClientsBuilderTest,
     // An error case: the zone has no configuration. (note .com here)
     const Command nozone_cmd(LOADZONE, Element::fromJSON(
                                  "{\"class\": \"IN\","
-                                 " \"origin\": \"example.com\"}"));
+                                 " \"origin\": \"example.com\"}"),
+                             FinishedCallback());
     EXPECT_THROW(builder.handleCommand(nozone_cmd),
                  TestDataSrcClientsBuilder::InternalCommandError);
     // The previous zone is not hurt in any way
@@ -403,7 +407,8 @@ TEST_F(DataSrcClientsBuilderTest,
     builder.handleCommand(
                      Command(LOADZONE, Element::fromJSON(
                                  "{\"class\": \"IN\","
-                                 " \"origin\": \"example.org\"}")));
+                                 " \"origin\": \"example.org\"}"),
+                             FinishedCallback()));
     // Only one mutex was needed because there was no actual reload.
     EXPECT_EQ(orig_lock_count + 1, map_mutex.lock_count);
     EXPECT_EQ(orig_unlock_count + 1, map_mutex.unlock_count);
@@ -421,7 +426,8 @@ TEST_F(DataSrcClientsBuilderTest,
         builder.handleCommand(
             Command(LOADZONE, Element::fromJSON(
                         "{\"class\": \"IN\","
-                        " \"origin\": \"nosuchzone.example\"}"))),
+                        " \"origin\": \"nosuchzone.example\"}"),
+                    FinishedCallback())),
         TestDataSrcClientsBuilder::InternalCommandError);
 
     // basically impossible case: in-memory cache is completely disabled.
@@ -441,7 +447,8 @@ TEST_F(DataSrcClientsBuilderTest,
     EXPECT_THROW(builder.handleCommand(
                      Command(LOADZONE, Element::fromJSON(
                                  "{\"class\": \"IN\","
-                                 " \"origin\": \"example.org\"}"))),
+                                 " \"origin\": \"example.org\"}"),
+                             FinishedCallback())),
                  TestDataSrcClientsBuilder::InternalCommandError);
 }
 
@@ -454,7 +461,8 @@ TEST_F(DataSrcClientsBuilderTest, loadBrokenZone) {
     // there's an error in the new zone file.  reload will be rejected.
     const Command loadzone_cmd(LOADZONE, Element::fromJSON(
                                    "{\"class\": \"IN\","
-                                   " \"origin\": \"test1.example\"}"));
+                                   " \"origin\": \"test1.example\"}"),
+                               FinishedCallback());
     EXPECT_THROW(builder.handleCommand(loadzone_cmd),
                  TestDataSrcClientsBuilder::InternalCommandError);
     zoneChecks(clients_map, rrclass);     // zone shouldn't be replaced
@@ -469,7 +477,8 @@ TEST_F(DataSrcClientsBuilderTest, loadUnreadableZone) {
                              TEST_DATA_BUILDDIR "/test1.zone.copied"));
     const Command loadzone_cmd(LOADZONE, Element::fromJSON(
                                    "{\"class\": \"IN\","
-                                   " \"origin\": \"test1.example\"}"));
+                                   " \"origin\": \"test1.example\"}"),
+                               FinishedCallback());
     EXPECT_THROW(builder.handleCommand(loadzone_cmd),
                  TestDataSrcClientsBuilder::InternalCommandError);
     zoneChecks(clients_map, rrclass);     // zone shouldn't be replaced
@@ -482,7 +491,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneWithoutDataSrc) {
                      Command(LOADZONE,
                              Element::fromJSON(
                                  "{\"class\": \"IN\", "
-                                 " \"origin\": \"test1.example\"}"))),
+                                 " \"origin\": \"test1.example\"}"),
+                             FinishedCallback())),
                  TestDataSrcClientsBuilder::InternalCommandError);
 }
 
@@ -492,7 +502,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
     if (!isc::util::unittests::runningOnValgrind()) {
         // null arg: this causes assertion failure
         EXPECT_DEATH_IF_SUPPORTED({
-                builder.handleCommand(Command(LOADZONE, ElementPtr()));
+                builder.handleCommand(Command(LOADZONE, ElementPtr(),
+                                              FinishedCallback()));
             }, "");
     }
 
@@ -501,7 +512,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
                      Command(LOADZONE,
                              Element::fromJSON(
                                  "{\"origin\": \"test1.example\","
-                                 " \"class\": \"no_such_class\"}"))),
+                                 " \"class\": \"no_such_class\"}"),
+                             FinishedCallback())),
                  InvalidRRClass);
 
     // not a string
@@ -509,7 +521,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
                      Command(LOADZONE,
                              Element::fromJSON(
                                  "{\"origin\": \"test1.example\","
-                                 " \"class\": 1}"))),
+                                 " \"class\": 1}"),
+                             FinishedCallback())),
                  isc::data::TypeError);
 
     // class or origin is missing: result in assertion failure
@@ -517,7 +530,8 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
         EXPECT_DEATH_IF_SUPPORTED({
                 builder.handleCommand(Command(LOADZONE,
                                               Element::fromJSON(
-                                                  "{\"class\": \"IN\"}")));
+                                                  "{\"class\": \"IN\"}"),
+                                              FinishedCallback()));
             }, "");
     }
 
@@ -525,12 +539,14 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
     EXPECT_THROW(builder.handleCommand(
                      Command(LOADZONE,
                              Element::fromJSON(
-                                 "{\"class\": \"IN\", \"origin\": \"...\"}"))),
+                                 "{\"class\": \"IN\", \"origin\": \"...\"}"),
+                             FinishedCallback())),
                  EmptyLabel);
     EXPECT_THROW(builder.handleCommand(
                      Command(LOADZONE,
                              Element::fromJSON(
-                                 "{\"origin\": 10, \"class\": 1}"))),
+                                 "{\"origin\": 10, \"class\": 1}"),
+                             FinishedCallback())),
                  isc::data::TypeError);
 }
 
@@ -561,7 +577,8 @@ TEST_F(DataSrcClientsBuilderTest,
                      Command(LOADZONE,
                              Element::fromJSON(
                                  "{\"origin\": \"test1.example\","
-                                 " \"class\": \"IN\"}"))),
+                                 " \"class\": \"IN\"}"),
+                             FinishedCallback())),
                  TestDataSrcClientsBuilder::InternalCommandError);
 }
 

src/bin/auth/tests/datasrc_clients_mgr_unittest.cc

@@ -43,8 +43,8 @@ shutdownCheck() {
     EXPECT_EQ(1, FakeDataSrcClientsBuilder::cond->signal_count);
     EXPECT_EQ(1, FakeDataSrcClientsBuilder::command_queue->size());
     const Command& cmd = FakeDataSrcClientsBuilder::command_queue->front();
-    EXPECT_EQ(SHUTDOWN, cmd.first);
-    EXPECT_FALSE(cmd.second);   // no argument
+    EXPECT_EQ(SHUTDOWN, cmd.id);
+    EXPECT_FALSE(cmd.params);   // no argument
 
     // Finally, the manager should wait for the thread to terminate.
     EXPECT_TRUE(FakeDataSrcClientsBuilder::thread_waited);
@@ -130,8 +130,8 @@ TEST(DataSrcClientsMgrTest, reconfigure) {
     // touch or refer to the map, so it shouldn't acquire the map lock.
     checkSharedMembers(1, 1, 0, 0, 1, 1);
     const Command& cmd1 = FakeDataSrcClientsBuilder::command_queue->front();
-    EXPECT_EQ(RECONFIGURE, cmd1.first);
-    EXPECT_EQ(reconfigure_arg, cmd1.second);
+    EXPECT_EQ(RECONFIGURE, cmd1.id);
+    EXPECT_EQ(reconfigure_arg, cmd1.params);
 
     // Non-null, but semantically invalid argument.  The manager doesn't do
     // this check, so it should result in the same effect.
@@ -140,8 +140,8 @@ TEST(DataSrcClientsMgrTest, reconfigure) {
     mgr.reconfigure(reconfigure_arg);
     checkSharedMembers(2, 2, 0, 0, 2, 1);
     const Command& cmd2 = FakeDataSrcClientsBuilder::command_queue->front();
-    EXPECT_EQ(RECONFIGURE, cmd2.first);
-    EXPECT_EQ(reconfigure_arg, cmd2.second);
+    EXPECT_EQ(RECONFIGURE, cmd2.id);
+    EXPECT_EQ(reconfigure_arg, cmd2.params);
 
     // Passing NULL argument is immediately rejected
     EXPECT_THROW(mgr.reconfigure(ConstElementPtr()), isc::InvalidParameter);

src/bin/auth/tests/test_datasrc_clients_mgr.cc

@@ -81,9 +81,9 @@ TestDataSrcClientsMgr::reconfigureHook() {
     using namespace datasrc_clientmgr_internal;
 
     // Simply replace the local map, ignoring bogus config value.
-    assert(command_queue_.front().first == RECONFIGURE);
+    assert(command_queue_.front().id == RECONFIGURE);
     try {
-        clients_map_ = configureDataSource(command_queue_.front().second);
+        clients_map_ = configureDataSource(command_queue_.front().params);
     } catch (...) {}
 }