Skip to content

Commit 697a0ad

Browse files
awlaneawlane
committed
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
1 parent a37efed commit 697a0ad

6 files changed

Lines changed: 66 additions & 43 deletions

File tree

‎README-dev.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ Boilerplate](https://redmine.named-data.net/projects/nlsr/wiki/NDN_Team_License_
3434
## Recommendations
3535

3636
NLSR code is subject to the code style defined
37-
[here](https://redmine.named-data.net/projects/nfd/wiki/CodeStyle).
37+
[here](https://docs.named-data.net/ndn-cxx/current/code-style.html).
3838

3939
NLSR Developer's guide can be found
4040
[here](https://github.com/named-data/NLSR/blob/developers-guide/NLSR-Developers-Guide.pdf).

‎src/update/command-processor.cpp‎

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -44,37 +44,43 @@ CommandProcessor::advertiseAndInsertPrefix(const ndn::mgmt::ControlParametersBas
4444
const ndn::mgmt::CommandContinuation& done)
4545
{
4646
const auto& castParams = static_cast<const ndn::nfd::ControlParameters&>(parameters);
47-
47+
// This is a bit of a hack, waiting on the work in #5348 to complete for full refactoring
48+
ndn::nfd::ControlParameters responseParams(castParams.wireEncode());
49+
uint64_t responseFaceId = (castParams.hasFaceId() && castParams.getFaceId() > 0)
50+
? responseParams.getFaceId() : m_defaultResponseFaceId;
51+
responseParams.setFaceId(responseFaceId);
4852
// Only build a Name LSA if the added name is new
4953
if (m_namePrefixList.insert(castParams.getName())) {
5054
NLSR_LOG_INFO("Advertising name: " << castParams.getName());
5155
m_lsdb.buildAndInstallOwnNameLsa();
5256
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
5357
NLSR_LOG_INFO("Saving name to the configuration file ");
54-
if (afterAdvertise(castParams.getName()) == true) {
55-
return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
58+
auto [afterAdvertiseReturn, afterAdvertiseMessage] = afterAdvertise(castParams.getName());
59+
if (afterAdvertiseReturn) {
60+
return done(ndn::nfd::ControlResponse(205, afterAdvertiseMessage).setBody(responseParams.wireEncode()));
5661
}
5762
else {
58-
return done(ndn::nfd::ControlResponse(406, "Failed to open configuration file.")
59-
.setBody(parameters.wireEncode()));
63+
return done(ndn::nfd::ControlResponse(500, afterAdvertiseMessage)
64+
.setBody(responseParams.wireEncode()));
6065
}
6166
}
62-
return done(ndn::nfd::ControlResponse(200, "OK").setBody(parameters.wireEncode()));
67+
return done(ndn::nfd::ControlResponse(200, "OK").setBody(responseParams.wireEncode()));
6368
}
6469
else {
6570
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
6671
// Save an already advertised prefix
6772
NLSR_LOG_INFO("Saving an already advertised name: " << castParams.getName());
68-
if (afterAdvertise(castParams.getName()) == true) {
69-
return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
73+
auto [afterAdvertiseReturn, afterAdvertiseMessage] = afterAdvertise(castParams.getName());
74+
if (afterAdvertiseReturn) {
75+
return done(ndn::nfd::ControlResponse(205, afterAdvertiseMessage).setBody(responseParams.wireEncode()));
7076
}
7177
else {
72-
return done(ndn::nfd::ControlResponse(406, "Prefix is already Saved/Failed to open configuration file.")
73-
.setBody(parameters.wireEncode()));
78+
return done(ndn::nfd::ControlResponse(500, afterAdvertiseMessage)
79+
.setBody(responseParams.wireEncode()));
7480
}
7581
}
7682
return done(ndn::nfd::ControlResponse(204, "Prefix is already advertised/inserted.")
77-
.setBody(parameters.wireEncode()));
83+
.setBody(responseParams.wireEncode()));
7884
}
7985
}
8086

@@ -83,36 +89,42 @@ CommandProcessor::withdrawAndRemovePrefix(const ndn::mgmt::ControlParametersBase
8389
const ndn::mgmt::CommandContinuation& done)
8490
{
8591
const auto& castParams = static_cast<const ndn::nfd::ControlParameters&>(parameters);
86-
92+
// This is a bit of a hack, waiting on the work in #5348 to complete for full refactoring
93+
ndn::nfd::ControlParameters responseParams(castParams.wireEncode());
94+
uint64_t responseFaceId = (castParams.hasFaceId() && castParams.getFaceId() > 0)
95+
? responseParams.getFaceId() : m_defaultResponseFaceId;
96+
responseParams.setFaceId(responseFaceId);
8797
// Only build a Name LSA if the added name is new
8898
if (m_namePrefixList.erase(castParams.getName())) {
8999
NLSR_LOG_INFO("Withdrawing/Removing name: " << castParams.getName());
90100
m_lsdb.buildAndInstallOwnNameLsa();
91101
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
92-
if (afterWithdraw(castParams.getName()) == true) {
93-
return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
102+
auto [afterWithdrawReturn, afterWithdrawMessage] = afterWithdraw(castParams.getName());
103+
if (afterWithdrawReturn) {
104+
return done(ndn::nfd::ControlResponse(205, afterWithdrawMessage).setBody(responseParams.wireEncode()));
94105
}
95106
else {
96-
return done(ndn::nfd::ControlResponse(406, "Failed to open configuration file.")
97-
.setBody(parameters.wireEncode()));
107+
return done(ndn::nfd::ControlResponse(500, afterWithdrawMessage)
108+
.setBody(responseParams.wireEncode()));
98109
}
99110
}
100-
return done(ndn::nfd::ControlResponse(200, "OK").setBody(parameters.wireEncode()));
111+
return done(ndn::nfd::ControlResponse(200, "OK").setBody(responseParams.wireEncode()));
101112
}
102113
else {
103114
if (castParams.hasFlags() && castParams.getFlags() == PREFIX_FLAG) {
104115
// Delete an already withdrawn prefix
105116
NLSR_LOG_INFO("Deleting an already withdrawn name: " << castParams.getName());
106-
if (afterWithdraw(castParams.getName()) == true) {
107-
return done(ndn::nfd::ControlResponse(205, "OK").setBody(parameters.wireEncode()));
117+
auto [afterWithdrawReturn, afterWithdrawMessage] = afterWithdraw(castParams.getName());
118+
if (afterWithdrawReturn) {
119+
return done(ndn::nfd::ControlResponse(205, afterWithdrawMessage).setBody(responseParams.wireEncode()));
108120
}
109121
else {
110-
return done(ndn::nfd::ControlResponse(406, "Prefix is already deleted/Failed to open configuration file.")
111-
.setBody(parameters.wireEncode()));
122+
return done(ndn::nfd::ControlResponse(500, afterWithdrawMessage)
123+
.setBody(responseParams.wireEncode()));
112124
}
113125
}
114126
return done(ndn::nfd::ControlResponse(204, "Prefix is already withdrawn/removed.")
115-
.setBody(parameters.wireEncode()));
127+
.setBody(responseParams.wireEncode()));
116128
}
117129
}
118130

‎src/update/command-processor.hpp‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -65,28 +65,33 @@ class CommandProcessor : boost::noncopyable
6565
withdrawAndRemovePrefix(const ndn::mgmt::ControlParametersBase& parameters,
6666
const ndn::mgmt::CommandContinuation& done);
6767

68-
/*! \brief Save an advertised prefix to the nlsr configuration file.
69-
* \return bool from the overridden function while nullopt here
68+
/*! \brief Processing after advertise command delegated to subclass.
69+
* This is always treated as successful if not implemented.
70+
* \return tuple {bool indicating success/failure, message string}.
7071
*/
71-
virtual std::optional<bool>
72+
virtual std::tuple<bool, std::string>
7273
afterAdvertise(const ndn::Name& prefix)
7374
{
74-
return std::nullopt;
75+
return {true, "OK"};
7576
}
7677

77-
/*! \brief Save an advertised prefix to the nlsr configuration file.
78-
* \return bool from the overridden function while nullopt here
78+
/*! \brief Processing after withdraw command delegated to subclass.
79+
* This is always treated as successful if not implemented.
80+
* \return tuple {bool indicating success/failure, message string}.
7981
*/
80-
virtual std::optional<bool>
82+
virtual std::tuple<bool, std::string>
8183
afterWithdraw(const ndn::Name& prefix)
8284
{
83-
return std::nullopt;
85+
return {true, "OK"};
8486
}
8587

8688
protected:
8789
ndn::mgmt::Dispatcher& m_dispatcher;
8890
NamePrefixList& m_namePrefixList;
8991
Lsdb& m_lsdb;
92+
93+
private:
94+
const uint64_t m_defaultResponseFaceId = 1;
9095
};
9196

9297
} // namespace nlsr::update

‎src/update/prefix-update-processor.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ PrefixUpdateProcessor::checkForPrefixInFile(const std::string prefix)
114114
return false;
115115
}
116116

117-
bool
117+
std::tuple<bool, std::string>
118118
PrefixUpdateProcessor::addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix)
119119
{
120120
std::string value = " prefix " + prefix.toUri();
@@ -124,14 +124,14 @@ PrefixUpdateProcessor::addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix
124124
std::fstream input(m_confFileNameDynamic, input.in);
125125
if (!input.good() || !input.is_open()) {
126126
NLSR_LOG_ERROR("Failed to open configuration file for parsing");
127-
return false;
127+
return {false, "Failed to open configuration file for parsing"};
128128
}
129129

130130
if (addPrefix) {
131131
//check if prefix already exist in the nlsr configuration file
132132
if (checkForPrefixInFile(value)) {
133133
NLSR_LOG_ERROR("Prefix already exists in the configuration file");
134-
return false;
134+
return {false, "Prefix already exists in the configuration file"};
135135
}
136136
while (!input.eof()) {
137137
getline(input, line);
@@ -147,7 +147,7 @@ PrefixUpdateProcessor::addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix
147147
else {
148148
if (!checkForPrefixInFile(value)) {
149149
NLSR_LOG_ERROR("Prefix doesn't exists in the configuration file");
150-
return false;
150+
return {false, "Prefix doesn't exists in the configuration file"};
151151
}
152152
boost::trim(value);
153153
while (!input.eof()) {
@@ -165,16 +165,16 @@ PrefixUpdateProcessor::addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix
165165
std::ofstream output(m_confFileNameDynamic);
166166
output << fileString;
167167
output.close();
168-
return true;
168+
return {true, "OK"};
169169
}
170170

171-
std::optional<bool>
171+
std::tuple<bool, std::string>
172172
PrefixUpdateProcessor::afterAdvertise(const ndn::Name& prefix)
173173
{
174174
return addOrDeletePrefix(prefix, true);
175175
}
176176

177-
std::optional<bool>
177+
std::tuple<bool, std::string>
178178
PrefixUpdateProcessor::afterWithdraw(const ndn::Name& prefix)
179179
{
180180
return addOrDeletePrefix(prefix, false);

‎src/update/prefix-update-processor.hpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,19 @@ class PrefixUpdateProcessor : public CommandProcessor
5757
/*! \brief Add or delete an advertise or withdrawn prefix to the nlsr
5858
* configuration file
5959
*/
60-
bool
60+
std::tuple<bool, std::string>
6161
addOrDeletePrefix(const ndn::Name& prefix, bool addPrefix);
6262

63-
std::optional<bool>
63+
/*! \brief Save an advertised prefix to the nlsr configuration file.
64+
* \return tuple {bool indicating success/failure, message string}.
65+
*/
66+
std::tuple<bool, std::string>
6467
afterAdvertise(const ndn::Name& prefix) override;
6568

66-
std::optional<bool>
69+
/*! \brief Remove an advertised prefix from the nlsr configuration file.
70+
* \return tuple {bool indicating success/failure, message string}.
71+
*/
72+
std::tuple<bool, std::string>
6773
afterWithdraw(const ndn::Name& prefix) override;
6874

6975
/*! \brief Check if a prefix exists in the nlsr configuration file */

‎tests/update/test-save-delete-prefix.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
22
/*
3-
* Copyright (c) 2014-2024, The University of Memphis,
3+
* Copyright (c) 2014-2025, The University of Memphis,
44
* Regents of the University of California,
55
* Arizona Board of Regents.
66
*
@@ -212,7 +212,7 @@ BOOST_AUTO_TEST_CASE(PrefixStillSavedAfterJustWithdrawn)
212212
face.receive(advertiseWithdraw("/prefix/to/save", "advertise", true));
213213
this->advanceClocks(ndn::time::milliseconds(10));
214214
BOOST_REQUIRE(counter == 1);
215-
BOOST_CHECK_EQUAL(getResponseCode(), 406);
215+
BOOST_CHECK_EQUAL(getResponseCode(), 500);
216216
face.sentData.clear();
217217

218218
// only withdraw

0 commit comments

Comments
 (0)