mgmt: Fix issue with readvertisement commands always returning error code
NFD RIB commands in NLSR were previously always returning 406 errors due
to not having correct default return values for optional post-processing.
This does not appear to impede functionality but does cause incorrect
behavior on the NFD side and is blocking other changes.
Also fixes an issue where NLSR was not sending correctly formatted responses
back to commands from NFD which was raising errors.
Refs #5358
Change-Id: Iffe6a2416a6d7eafa44e17c821bf903c12ecb104
diff --git a/src/update/command-processor.cpp b/src/update/command-processor.cpp
index d13d9b6..67f58cc 100644
--- a/src/update/command-processor.cpp
+++ b/src/update/command-processor.cpp
@@ -44,37 +44,43 @@
const ndn::mgmt::CommandContinuation& done)
{
const auto& castParams = static_cast<const ndn::nfd::ControlParameters&>(parameters);
-
+ // This is a bit of a hack, waiting on the work in #5348 to complete for full refactoring
+ ndn::nfd::ControlParameters responseParams(castParams.wireEncode());
+ uint64_t responseFaceId = (castParams.hasFaceId() && castParams.getFaceId() > 0)
+ ? responseParams.getFaceId() : m_defaultResponseFaceId;
+ responseParams.setFaceId(responseFaceId);
// Only build a Name LSA if the added name is new
if (m_namePrefixList.insert(castParams.getName())) {
NLSR_LOG_INFO("Advertising name: " << castParams.getName());
m_lsdb.buildAndInstallOwnNameLsa();
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
NLSR_LOG_INFO("Saving name to the configuration file ");
- if (afterAdvertise(castParams.getName()) == true) {
- return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
+ auto [afterAdvertiseReturn, afterAdvertiseMessage] = afterAdvertise(castParams.getName());
+ if (afterAdvertiseReturn) {
+ return done(ndn::nfd::ControlResponse(205, afterAdvertiseMessage).setBody(responseParams.wireEncode()));
}
else {
- return done(ndn::nfd::ControlResponse(406, "Failed to open configuration file.")
- .setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(500, afterAdvertiseMessage)
+ .setBody(responseParams.wireEncode()));
}
}
- return done(ndn::nfd::ControlResponse(200, "OK").setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(200, "OK").setBody(responseParams.wireEncode()));
}
else {
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
// Save an already advertised prefix
NLSR_LOG_INFO("Saving an already advertised name: " << castParams.getName());
- if (afterAdvertise(castParams.getName()) == true) {
- return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
+ auto [afterAdvertiseReturn, afterAdvertiseMessage] = afterAdvertise(castParams.getName());
+ if (afterAdvertiseReturn) {
+ return done(ndn::nfd::ControlResponse(205, afterAdvertiseMessage).setBody(responseParams.wireEncode()));
}
else {
- return done(ndn::nfd::ControlResponse(406, "Prefix is already Saved/Failed to open configuration file.")
- .setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(500, afterAdvertiseMessage)
+ .setBody(responseParams.wireEncode()));
}
}
return done(ndn::nfd::ControlResponse(204, "Prefix is already advertised/inserted.")
- .setBody(parameters.wireEncode()));
+ .setBody(responseParams.wireEncode()));
}
}
@@ -83,36 +89,42 @@
const ndn::mgmt::CommandContinuation& done)
{
const auto& castParams = static_cast<const ndn::nfd::ControlParameters&>(parameters);
-
+ // This is a bit of a hack, waiting on the work in #5348 to complete for full refactoring
+ ndn::nfd::ControlParameters responseParams(castParams.wireEncode());
+ uint64_t responseFaceId = (castParams.hasFaceId() && castParams.getFaceId() > 0)
+ ? responseParams.getFaceId() : m_defaultResponseFaceId;
+ responseParams.setFaceId(responseFaceId);
// Only build a Name LSA if the added name is new
if (m_namePrefixList.erase(castParams.getName())) {
NLSR_LOG_INFO("Withdrawing/Removing name: " << castParams.getName());
m_lsdb.buildAndInstallOwnNameLsa();
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
- if (afterWithdraw(castParams.getName()) == true) {
- return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
+ auto [afterWithdrawReturn, afterWithdrawMessage] = afterWithdraw(castParams.getName());
+ if (afterWithdrawReturn) {
+ return done(ndn::nfd::ControlResponse(205, afterWithdrawMessage).setBody(responseParams.wireEncode()));
}
else {
- return done(ndn::nfd::ControlResponse(406, "Failed to open configuration file.")
- .setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(500, afterWithdrawMessage)
+ .setBody(responseParams.wireEncode()));
}
}
- return done(ndn::nfd::ControlResponse(200, "OK").setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(200, "OK").setBody(responseParams.wireEncode()));
}
else {
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
// Delete an already withdrawn prefix
NLSR_LOG_INFO("Deleting an already withdrawn name: " << castParams.getName());
- if (afterWithdraw(castParams.getName()) == true) {
- return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
+ auto [afterWithdrawReturn, afterWithdrawMessage] = afterWithdraw(castParams.getName());
+ if (afterWithdrawReturn) {
+ return done(ndn::nfd::ControlResponse(205, afterWithdrawMessage).setBody(responseParams.wireEncode()));
}
else {
- return done(ndn::nfd::ControlResponse(406, "Prefix is already deleted/Failed to open configuration file.")
- .setBody(parameters.wireEncode()));
+ return done(ndn::nfd::ControlResponse(500, afterWithdrawMessage)
+ .setBody(responseParams.wireEncode()));
}
}
return done(ndn::nfd::ControlResponse(204, "Prefix is already withdrawn/removed.")
- .setBody(parameters.wireEncode()));
+ .setBody(responseParams.wireEncode()));
}
}
diff --git a/src/update/command-processor.hpp b/src/update/command-processor.hpp
index d0245fd..1d25ec0 100644
--- a/src/update/command-processor.hpp
+++ b/src/update/command-processor.hpp
@@ -65,28 +65,33 @@
withdrawAndRemovePrefix(const ndn::mgmt::ControlParametersBase& parameters,
const ndn::mgmt::CommandContinuation& done);
- /*! \brief Save an advertised prefix to the nlsr configuration file.
- * \return bool from the overridden function while nullopt here
+ /*! \brief Processing after advertise command delegated to subclass.
+ * This is always treated as successful if not implemented.
+ * \return tuple {bool indicating success/failure, message string}.
*/
- virtual std::optional<bool>
+ virtual std::tuple<bool, std::string>
afterAdvertise(const ndn::Name& prefix)
{
- return std::nullopt;
+ return {true, "OK"};
}
- /*! \brief Save an advertised prefix to the nlsr configuration file.
- * \return bool from the overridden function while nullopt here
+ /*! \brief Processing after withdraw command delegated to subclass.
+ * This is always treated as successful if not implemented.
+ * \return tuple {bool indicating success/failure, message string}.
*/
- virtual std::optional<bool>
+ virtual std::tuple<bool, std::string>
afterWithdraw(const ndn::Name& prefix)
{
- return std::nullopt;
+ return {true, "OK"};
}
protected:
ndn::mgmt::Dispatcher& m_dispatcher;
NamePrefixList& m_namePrefixList;
Lsdb& m_lsdb;
+
+private:
+ const uint64_t m_defaultResponseFaceId = 1;
};
} // namespace nlsr::update
diff --git a/src/update/prefix-update-processor.cpp b/src/update/prefix-update-processor.cpp
index f07f0d0..2f403fb 100644
--- a/src/update/prefix-update-processor.cpp
+++ b/src/update/prefix-update-processor.cpp
@@ -114,7 +114,7 @@
return false;
}
-bool
+std::tuple<bool, std::string>
PrefixUpdateProcessor::addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix)
{
std::string value = " prefix " + prefix.toUri();
@@ -124,14 +124,14 @@
std::fstream input(m_confFileNameDynamic, input.in);
if (!input.good() || !input.is_open()) {
NLSR_LOG_ERROR("Failed to open configuration file for parsing");
- return false;
+ return {false, "Failed to open configuration file for parsing"};
}
if (addPrefix) {
//check if prefix already exist in the nlsr configuration file
if (checkForPrefixInFile(value)) {
NLSR_LOG_ERROR("Prefix already exists in the configuration file");
- return false;
+ return {false, "Prefix already exists in the configuration file"};
}
while (!input.eof()) {
getline(input, line);
@@ -147,7 +147,7 @@
else {
if (!checkForPrefixInFile(value)) {
NLSR_LOG_ERROR("Prefix doesn't exists in the configuration file");
- return false;
+ return {false, "Prefix doesn't exists in the configuration file"};
}
boost::trim(value);
while (!input.eof()) {
@@ -165,16 +165,16 @@
std::ofstream output(m_confFileNameDynamic);
output << fileString;
output.close();
- return true;
+ return {true, "OK"};
}
-std::optional<bool>
+std::tuple<bool, std::string>
PrefixUpdateProcessor::afterAdvertise(const ndn::Name& prefix)
{
return addOrDeletePrefix(prefix, true);
}
-std::optional<bool>
+std::tuple<bool, std::string>
PrefixUpdateProcessor::afterWithdraw(const ndn::Name& prefix)
{
return addOrDeletePrefix(prefix, false);
diff --git a/src/update/prefix-update-processor.hpp b/src/update/prefix-update-processor.hpp
index bc59937..bc05937 100644
--- a/src/update/prefix-update-processor.hpp
+++ b/src/update/prefix-update-processor.hpp
@@ -57,13 +57,19 @@
/*! \brief Add or delete an advertise or withdrawn prefix to the nlsr
* configuration file
*/
- bool
+ std::tuple<bool, std::string>
addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix);
- std::optional<bool>
+ /*! \brief Save an advertised prefix to the nlsr configuration file.
+ * \return tuple {bool indicating success/failure, message string}.
+ */
+ std::tuple<bool, std::string>
afterAdvertise(const ndn::Name& prefix) override;
- std::optional<bool>
+ /*! \brief Remove an advertised prefix from the nlsr configuration file.
+ * \return tuple {bool indicating success/failure, message string}.
+ */
+ std::tuple<bool, std::string>
afterWithdraw(const ndn::Name& prefix) override;
/*! \brief Check if a prefix exists in the nlsr configuration file */