Change the logic of generating the D-Bus object identifier
Presently D-Bus object identifier is generated with the hash of
D-Bus properties, as a result of this if the value of D-Bus property
gets change the D-Bus object identifier will get change.
This commit fixes this problem by keeping the last identifier
value in the manager object so whenever user creates new snmp
manager object it is incremented by 1, hence the object path
becomes unique.
This commit also takes care of that if at any point of time
if snmp manager app restart it gets the same D-Bus identifier
as it was before the restart.
Change-Id: I456c11f7824ff678ae470bc6641f0e0cc7c1f6bc
Signed-off-by: Ratan Gupta <ratagupt@linux.vnet.ibm.com>
diff --git a/snmp_client.cpp b/snmp_client.cpp
index 4049e2d..2d2e1d5 100644
--- a/snmp_client.cpp
+++ b/snmp_client.cpp
@@ -11,6 +11,7 @@
Client::Client(sdbusplus::bus::bus& bus, const char* objPath,
ConfManager& parent, const std::string& address, uint16_t port) :
Ifaces(bus, objPath, true),
+ id(std::stol(std::experimental::filesystem::path(objPath).filename())),
parent(parent)
{
this->address(address);
@@ -22,7 +23,7 @@
void Client::delete_()
{
- parent.deleteSNMPClient(this->address());
+ parent.deleteSNMPClient(id);
}
} // namespace snmp
diff --git a/snmp_client.hpp b/snmp_client.hpp
index fbc76fb..aab72cc 100644
--- a/snmp_client.hpp
+++ b/snmp_client.hpp
@@ -1,4 +1,5 @@
#pragma once
+#include <experimental/filesystem>
#include "xyz/openbmc_project/Network/Client/server.hpp"
#include "xyz/openbmc_project/Object/Delete/server.hpp"
@@ -21,6 +22,8 @@
sdbusplus::xyz::openbmc_project::Network::server::Client,
sdbusplus::xyz::openbmc_project::Object::server::Delete>;
+using Id = size_t;
+
/** @class Client
* @brief represents the snmp client configuration
* @details A concrete implementation for the
@@ -52,7 +55,9 @@
* @param[in] parent - Parent D-bus Object.
*/
Client(sdbusplus::bus::bus &bus, const char *objPath, ConfManager &parent) :
- Ifaces(bus, objPath, true), parent(parent)
+ Ifaces(bus, objPath, true),
+ id(std::stol(std::experimental::filesystem::path(objPath).filename())),
+ parent(parent)
{
}
@@ -61,6 +66,8 @@
void delete_() override;
private:
+ /** Client ID. */
+ Id id;
/** @brief Parent D-Bus Object. */
ConfManager &parent;
};
diff --git a/snmp_conf_manager.cpp b/snmp_conf_manager.cpp
index a215ef4..b523c5d 100644
--- a/snmp_conf_manager.cpp
+++ b/snmp_conf_manager.cpp
@@ -31,13 +31,9 @@
std::string ConfManager::client(std::string address, uint16_t port)
{
- auto clientEntry = this->clients.find(address);
- if (clientEntry != this->clients.end())
- {
- log<level::ERR>("Client already configured"),
- entry("ADDRESS=%s", address.c_str());
- elog<InternalFailure>();
- }
+ // TODO: Check whether the given manager is already there or not.
+
+ lastClientId++;
try
{
// just to check whether given address is valid or not.
@@ -51,45 +47,35 @@
Argument::ARGUMENT_VALUE(address.c_str()));
}
+ // create the D-Bus object
std::experimental::filesystem::path objPath;
objPath /= objectPath;
- objPath /= generateId(address, port);
- // create the D-Bus object
+ objPath /= std::to_string(lastClientId);
+
auto client = std::make_unique<phosphor::network::snmp::Client>(
bus, objPath.string().c_str(), *this, address, port);
- // save the D-Bus object
- serialize(*client, dbusPersistentLocation);
- this->clients.emplace(address, std::move(client));
+ // save the D-Bus object
+ serialize(lastClientId, *client, dbusPersistentLocation);
+
+ this->clients.emplace(lastClientId, std::move(client));
return objPath.string();
}
-std::string ConfManager::generateId(const std::string& address, uint16_t port)
+void ConfManager::deleteSNMPClient(Id id)
{
- std::stringstream hexId;
- std::string hashString = address;
- hashString += std::to_string(port);
-
- // Only want 8 hex digits.
- hexId << std::hex << ((std::hash<std::string>{}(hashString)) & 0xFFFFFFFF);
- return hexId.str();
-}
-
-void ConfManager::deleteSNMPClient(const std::string& address)
-{
- auto it = clients.find(address);
+ auto it = clients.find(id);
if (it == clients.end())
{
log<level::ERR>("Unable to delete the snmp client.",
- entry("ADDRESS=%s", address.c_str()));
+ entry("ID=%d", id));
return;
}
std::error_code ec;
// remove the persistent file
fs::path fileName = dbusPersistentLocation;
- fileName /=
- it->second->address() + SEPARATOR + std::to_string(it->second->port());
+ fileName /= std::to_string(id);
if (fs::exists(fileName))
{
@@ -126,19 +112,20 @@
}
auto managerID = confFile.path().filename().string();
- auto pos = managerID.find(SEPARATOR);
- auto ipaddress = managerID.substr(0, pos);
- auto port_str = managerID.substr(pos + 1);
- uint16_t port = stoi(port_str, nullptr);
+ Id idNum = std::stol(managerID);
fs::path objPath = objectPath;
- objPath /= generateId(ipaddress, port);
+ objPath /= managerID;
auto manager =
std::make_unique<Client>(bus, objPath.string().c_str(), *this);
if (deserialize(confFile.path(), *manager))
{
manager->emit_object_added();
- this->clients.emplace(ipaddress, std::move(manager));
+ this->clients.emplace(idNum, std::move(manager));
+ if (idNum > lastClientId)
+ {
+ lastClientId = idNum;
+ }
}
}
}
diff --git a/snmp_conf_manager.hpp b/snmp_conf_manager.hpp
index 7381314..f477b31 100644
--- a/snmp_conf_manager.hpp
+++ b/snmp_conf_manager.hpp
@@ -15,8 +15,7 @@
namespace snmp
{
-using IPAddress = std::string;
-using ClientList = std::map<IPAddress, std::unique_ptr<Client>>;
+using ClientList = std::map<Id, std::unique_ptr<Client>>;
namespace fs = std::experimental::filesystem;
namespace details
@@ -54,10 +53,10 @@
*/
std::string client(std::string address, uint16_t port) override;
- /* @brief delete the D-Bus object of the given ipaddress.
- * @param[in] address - IP address/Hostname.
+ /* @brief delete the D-Bus object of the given ID.
+ * @param[in] id - client identifier.
*/
- void deleteSNMPClient(const std::string& address);
+ void deleteSNMPClient(Id id);
/** @brief Construct manager/client D-Bus objects from their persisted
* representations.
@@ -67,14 +66,6 @@
/** @brief location of the persisted D-Bus object.*/
fs::path dbusPersistentLocation;
- protected:
- /** @brief generates the id by doing hash of ipaddress, port
- * @param[in] address - IP address/Hostname.
- * @param[in] port - network port.
- * @return hash string.
- */
- static std::string generateId(const std::string& address, uint16_t port);
-
private:
/** @brief sdbusplus DBus bus object. */
sdbusplus::bus::bus& bus;
@@ -82,9 +73,12 @@
/** @brief Path of Object. */
std::string objectPath;
- /** @brief map of IPAddress dbus objects and their names */
+ /** @brief map of SNMP Client dbus objects and their ID */
ClientList clients;
+ /** @brief Id of the last SNMP manager entry */
+ Id lastClientId = 0;
+
friend class TestSNMPConfManager;
};
diff --git a/snmp_serialize.cpp b/snmp_serialize.cpp
index 2bc8f84..98aa97f 100644
--- a/snmp_serialize.cpp
+++ b/snmp_serialize.cpp
@@ -54,13 +54,11 @@
manager.port(port);
}
-fs::path serialize(const Client& manager, const fs::path& dir)
+fs::path serialize(Id id, const Client& manager, const fs::path& dir)
{
fs::path fileName = dir;
fs::create_directories(dir);
- auto address = manager.address();
- auto port = manager.port();
- fileName /= address + SEPARATOR + std::to_string(port);
+ fileName /= std::to_string(id);
std::ofstream os(fileName.string(), std::ios::binary);
cereal::BinaryOutputArchive oarchive(os);
diff --git a/snmp_serialize.hpp b/snmp_serialize.hpp
index 2031e22..2608866 100644
--- a/snmp_serialize.hpp
+++ b/snmp_serialize.hpp
@@ -14,13 +14,14 @@
namespace fs = std::experimental::filesystem;
-/** @brief Serialize and persist SNMP manager/client D-Bus object
+/** @brief Serialize and persist SNMP manager/client D-Bus object.
+ * @param[in] id - filename of the persisted SNMP manager object.
* @param[in] manager - const reference to snmp client/manager object.
* @param[in] path - path of persistent location where D-Bus object would be
* saved.
* @return fs::path - pathname of persisted snmp manager/client file.
*/
-fs::path serialize(const Client& manager, const fs::path& path);
+fs::path serialize(Id id, const Client& manager, const fs::path& path);
/** @brief Deserialze SNMP manager/client info into a D-Bus object
* @param[in] path - pathname of persisted manager/client file.
diff --git a/test/test_snmp_conf_manager.cpp b/test/test_snmp_conf_manager.cpp
index a681f21..f6111f7 100644
--- a/test/test_snmp_conf_manager.cpp
+++ b/test/test_snmp_conf_manager.cpp
@@ -12,13 +12,16 @@
namespace snmp
{
+auto managerObjPath = "/xyz/openbmc_test/snmp/manager";
+
class TestSNMPConfManager : public testing::Test
{
public:
sdbusplus::bus::bus bus;
ConfManager manager;
std::string confDir;
- TestSNMPConfManager() : bus(sdbusplus::bus::new_default()), manager(bus, "")
+ TestSNMPConfManager() :
+ bus(sdbusplus::bus::new_default()), manager(bus, managerObjPath)
{
char tmp[] = "/tmp/snmpManager.XXXXXX";
std::string confDir = mkdtemp(tmp);
@@ -30,9 +33,9 @@
fs::remove_all(manager.dbusPersistentLocation);
}
- void createSNMPClient(std::string ipaddress, uint16_t port)
+ std::string createSNMPClient(std::string ipaddress, uint16_t port)
{
- manager.client(ipaddress, port);
+ return manager.client(ipaddress, port);
}
ClientList &getSNMPClients()
@@ -40,52 +43,86 @@
return manager.clients;
}
+ bool isClientExist(const std::string &ipaddress)
+ {
+ for (const auto &val : manager.clients)
+ {
+ if (val.second.get()->address() == ipaddress)
+ {
+ return true;
+ }
+ }
+ return false;
+ }
+
void deleteSNMPClient(std::string ipaddress)
{
- auto &it = manager.clients[ipaddress];
- it->delete_();
+ for (const auto &val : manager.clients)
+ {
+ if (val.second.get()->address() == ipaddress)
+ {
+ val.second.get()->delete_();
+ }
+ }
}
};
// Add single SNMP client
TEST_F(TestSNMPConfManager, AddSNMPClient)
{
- using namespace sdbusplus::xyz::openbmc_project::Common::Error;
+ // check the created object path
+ auto path = createSNMPClient("192.168.1.1", 24);
+ std::string expectedPath = managerObjPath;
+ expectedPath += std::string("/1");
- createSNMPClient("192.168.1.1", 24);
+ EXPECT_EQ(path, expectedPath);
+ // check whether the client created
auto &clients = getSNMPClients();
EXPECT_EQ(1, clients.size());
- EXPECT_EQ(true, clients.find("192.168.1.1") != clients.end());
+ EXPECT_EQ(true, isClientExist("192.168.1.1"));
}
// Add multiple SNMP client
TEST_F(TestSNMPConfManager, AddMultipleSNMPClient)
{
- using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-
+ // add multiple clients and check whether the object path is generated
+ // correctly.
createSNMPClient("192.168.1.1", 24);
- createSNMPClient("192.168.1.2", 24);
+ auto path = createSNMPClient("192.168.1.2", 24);
+ std::string expectedPath = managerObjPath;
+ expectedPath += std::string("/2");
+ EXPECT_EQ(path, expectedPath);
+
+ // check both the clients get created
auto &clients = getSNMPClients();
EXPECT_EQ(2, clients.size());
- EXPECT_EQ(true, clients.find("192.168.1.1") != clients.end());
- EXPECT_EQ(true, clients.find("192.168.1.2") != clients.end());
+
+ EXPECT_EQ(true, isClientExist("192.168.1.1"));
+ EXPECT_EQ(true, isClientExist("192.168.1.2"));
}
// Delete SNMP client
TEST_F(TestSNMPConfManager, DeleteSNMPClient)
{
- using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-
createSNMPClient("192.168.1.1", 24);
createSNMPClient("192.168.1.2", 24);
auto &clients = getSNMPClients();
EXPECT_EQ(2, clients.size());
+
deleteSNMPClient("192.168.1.1");
- EXPECT_EQ(1, clients.size());
- EXPECT_EQ(true, clients.find("192.168.1.2") != clients.end());
+
+ auto path = createSNMPClient("192.168.1.3", 24);
+ std::string expectedPath = managerObjPath;
+ expectedPath += std::string("/3");
+ EXPECT_EQ(path, expectedPath);
+
+ EXPECT_EQ(2, clients.size());
+ EXPECT_EQ(true, isClientExist("192.168.1.2"));
+ EXPECT_EQ(false, isClientExist("192.168.1.1"));
+ EXPECT_EQ(true, isClientExist("192.168.1.3"));
}
} // namespace snmp
diff --git a/test/test_snmp_serialize.cpp b/test/test_snmp_serialize.cpp
index 60a51aa..df27b5b 100644
--- a/test/test_snmp_serialize.cpp
+++ b/test/test_snmp_serialize.cpp
@@ -39,10 +39,13 @@
TEST_F(TestSerialize, serialize)
{
- Client client(bus, clientObjPath, manager, "1.1.1.1", 23);
+ std::string objPath = clientObjPath;
+ objPath += "/" + std::to_string(1);
- auto path = serialize(client, manager.dbusPersistentLocation);
- Client restoreClient(bus, clientObjPath, manager);
+ Client client(bus, objPath.c_str(), manager, "1.1.1.1", 23);
+
+ auto path = serialize(1, client, manager.dbusPersistentLocation);
+ Client restoreClient(bus, objPath.c_str(), manager);
deserialize(path, restoreClient);
@@ -52,9 +55,12 @@
TEST_F(TestSerialize, deserialize_non_existent_file)
{
- Client client(bus, clientObjPath, manager);
+ std::string objPath = clientObjPath;
+ objPath += "/" + std::to_string(1);
+
+ Client client(bus, objPath.c_str(), manager);
fs::path path = manager.dbusPersistentLocation;
- path /= "snmpTest";
+ path /= "1";
auto ret = deserialize(path, client);
@@ -63,12 +69,15 @@
TEST_F(TestSerialize, deserialize_empty_file)
{
- Client restoreClient(bus, clientObjPath, manager);
+ std::string objPath = clientObjPath;
+ objPath += "/" + std::to_string(1);
+
+ Client restoreClient(bus, objPath.c_str(), manager);
std::fstream file;
fs::path path = manager.dbusPersistentLocation;
- path /= "snmpTest";
+ path /= "1";
file.open(path.string(), std::ofstream::out);
file.close();