mgmt+tools: rework FacePersistency handling in faces/create and faces/update
1. enable changing persistency in faces/update command
2. reject faces/create request if the face already exists
3. nfdc changes to support the above
Change-Id: I659906da846608a42a768f08fb110ceee1a947a7
refs: #3232
diff --git a/daemon/face/udp-channel.cpp b/daemon/face/udp-channel.cpp
index 64907be..b89a8ad 100644
--- a/daemon/face/udp-channel.cpp
+++ b/daemon/face/udp-channel.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -147,17 +147,7 @@
auto it = m_channelFaces.find(remoteEndpoint);
if (it != m_channelFaces.end()) {
// we already have a face for this endpoint, so reuse it
- auto face = it->second;
-
- // TODO #3232: Remove persistency transitions from faces/create
- // only on-demand -> persistent -> permanent transition is allowed
- bool isTransitionAllowed = persistency != face->getPersistency() &&
- (face->getPersistency() == ndn::nfd::FACE_PERSISTENCY_ON_DEMAND ||
- persistency == ndn::nfd::FACE_PERSISTENCY_PERMANENT);
- if (isTransitionAllowed) {
- face->setPersistency(persistency);
- }
- return {false, face};
+ return {false, it->second};
}
// else, create a new face
@@ -170,8 +160,6 @@
auto transport = make_unique<face::UnicastUdpTransport>(std::move(socket), persistency, m_idleFaceTimeout);
auto face = make_shared<Face>(std::move(linkService), std::move(transport));
- face->setPersistency(persistency);
-
m_channelFaces[remoteEndpoint] = face;
connectFaceClosedSignal(*face,
[this, remoteEndpoint] {
diff --git a/daemon/mgmt/face-manager.cpp b/daemon/mgmt/face-manager.cpp
index 4d1929d..b66a939 100644
--- a/daemon/mgmt/face-manager.cpp
+++ b/daemon/mgmt/face-manager.cpp
@@ -41,6 +41,7 @@
, m_faceSystem(faceSystem)
, m_faceTable(faceSystem.getFaceTable())
{
+ // register handlers for ControlCommand
registerCommandHandler<ndn::nfd::FaceCreateCommand>("create",
bind(&FaceManager::createFace, this, _2, _3, _4, _5));
@@ -56,10 +57,12 @@
registerCommandHandler<ndn::nfd::FaceDisableLocalControlCommand>("disable-local-control",
bind(&FaceManager::disableLocalControl, this, _2, _3, _4, _5));
+ // register handlers for StatusDataset
registerStatusDatasetHandler("list", bind(&FaceManager::listFaces, this, _1, _2, _3));
registerStatusDatasetHandler("channels", bind(&FaceManager::listChannels, this, _1, _2, _3));
registerStatusDatasetHandler("query", bind(&FaceManager::queryFaces, this, _1, _2, _3));
+ // register notification stream
m_postNotification = registerNotificationStream("events");
m_faceAddConn = m_faceTable.afterAdd.connect([this] (const Face& face) {
connectFaceStateChangeSignal(face);
@@ -121,34 +124,20 @@
}
}
-/**
- * \todo #3232
- * If the creation of this face would conflict with an existing face (e.g. same underlying
- * protocol and remote address, or a NIC-associated permanent face), the command will fail
- * with StatusCode 409.
- */
void
FaceManager::afterCreateFaceSuccess(const ControlParameters& parameters,
const shared_ptr<Face>& face,
const ndn::mgmt::CommandContinuation& done)
{
- // TODO: Re-enable check in #3232
- //if (face->getId() != face::INVALID_FACEID) {
- //// Face already exists
- //ControlParameters response;
- //response.setFaceId(face->getId());
- //response.setUri(face->getRemoteUri().toString());
- //response.setFacePersistency(face->getPersistency());
- //
- //auto linkService = dynamic_cast<face::GenericLinkService*>(face->getLinkService());
- //BOOST_ASSERT(linkService != nullptr);
- //auto options = linkService->getOptions();
- //response.setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, options.allowLocalFields, false);
- //
- // NFD_LOG_TRACE("Attempted to create duplicate face of " << face->getId());
- // done(ControlResponse(409, "Face with remote URI already exists").setBody(response.wireEncode()));
- //}
- //else {
+ if (face->getId() != face::INVALID_FACEID) {// Face already exists
+ NFD_LOG_TRACE("Attempted to create duplicate face of " << face->getId());
+
+ ControlParameters response = collectFaceProperties(*face);
+ response.setUri(face->getRemoteUri().toString());
+ done(ControlResponse(409, "Face with remote URI already exists").setBody(response.wireEncode()));
+ return;
+ }
+
// If scope non-local and flags set to enable local fields, request shouldn't
// have made it this far
BOOST_ASSERT(face->getScope() == ndn::nfd::FACE_SCOPE_LOCAL ||
@@ -158,16 +147,8 @@
m_faceTable.add(face);
- ControlParameters response;
- response.setFaceId(face->getId());
- response.setFacePersistency(face->getPersistency());
- response.setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED,
- parameters.hasFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) ?
- parameters.getFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) : false,
- false);
-
+ ControlParameters response = collectFaceProperties(*face);
done(ControlResponse(200, "OK").setBody(response.wireEncode()));
- //}
}
void
@@ -209,13 +190,6 @@
ControlParameters response;
bool areParamsValid = true;
- if (parameters.hasFacePersistency()) {
- // TODO #3232: Add FacePersistency updating
- NFD_LOG_TRACE("received unsupported face persistency change");
- areParamsValid = false;
- response.setFacePersistency(parameters.getFacePersistency());
- }
-
if (parameters.hasFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) &&
parameters.getFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) &&
face->getScope() != ndn::nfd::FACE_SCOPE_LOCAL) {
@@ -225,24 +199,29 @@
parameters.getFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED));
}
+ // check whether the requested FacePersistency change is valid if it's present
+ if (parameters.hasFacePersistency()) {
+ auto persistency = parameters.getFacePersistency();
+ if (!face->getTransport()->canChangePersistencyTo(persistency)) {
+ NFD_LOG_TRACE("cannot change face persistency to " << persistency);
+ areParamsValid = false;
+ response.setFacePersistency(persistency);
+ }
+ }
+
if (!areParamsValid) {
done(ControlResponse(409, "Invalid properties specified").setBody(response.wireEncode()));
return;
}
// All specified properties are valid, so make changes
-
- // TODO #3232: Add FacePersistency updating
-
+ if (parameters.hasFacePersistency()) {
+ face->setPersistency(parameters.getFacePersistency());
+ }
setLinkServiceOptions(*face, parameters, response);
// Set ControlResponse fields
- response.setFaceId(faceId);
- response.setFacePersistency(face->getPersistency());
- response.setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED,
- parameters.hasFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) ?
- parameters.getFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) : false,
- false);
+ response = collectFaceProperties(*face);
done(ControlResponse(200, "OK").setBody(response.wireEncode()));
}
@@ -354,6 +333,19 @@
response.setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, options.allowLocalFields, false);
}
+ControlParameters
+FaceManager::collectFaceProperties(const Face& face)
+{
+ auto linkService = dynamic_cast<face::GenericLinkService*>(face.getLinkService());
+ BOOST_ASSERT(linkService != nullptr);
+ auto options = linkService->getOptions();
+
+ return ControlParameters()
+ .setFaceId(face.getId())
+ .setFacePersistency(face.getPersistency())
+ .setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, options.allowLocalFields, false);
+}
+
void
FaceManager::listFaces(const Name& topPrefix, const Interest& interest,
ndn::mgmt::StatusDatasetContext& context)
diff --git a/daemon/mgmt/face-manager.hpp b/daemon/mgmt/face-manager.hpp
index 9a8c503..b7991b8 100644
--- a/daemon/mgmt/face-manager.hpp
+++ b/daemon/mgmt/face-manager.hpp
@@ -103,6 +103,9 @@
const ControlParameters& parameters,
ControlParameters& response);
+ static ControlParameters
+ collectFaceProperties(const Face& face);
+
PUBLIC_WITH_TESTS_ELSE_PRIVATE: // StatusDataset
void
listFaces(const Name& topPrefix, const Interest& interest,
diff --git a/tests/daemon/mgmt/face-manager-command-fixture.cpp b/tests/daemon/mgmt/face-manager-command-fixture.cpp
index 989ff7b..8eecf9e 100644
--- a/tests/daemon/mgmt/face-manager-command-fixture.cpp
+++ b/tests/daemon/mgmt/face-manager-command-fixture.cpp
@@ -85,6 +85,24 @@
}
}
+const Face*
+FaceManagerCommandNode::findFaceByUri(const std::string& uri) const
+{
+ for (const auto& face : faceTable) {
+ if (face.getRemoteUri().toString() == uri) {
+ return &face;
+ }
+ }
+ return nullptr;
+}
+
+FaceId
+FaceManagerCommandNode::findFaceIdByUri(const std::string& uri) const
+{
+ auto face = findFaceByUri(uri);
+ return face != nullptr ? face->getId() : face::INVALID_FACEID;
+}
+
FaceManagerCommandFixture::FaceManagerCommandFixture()
: node1(m_keyChain, 16363)
, node2(m_keyChain, 26363)
diff --git a/tests/daemon/mgmt/face-manager-command-fixture.hpp b/tests/daemon/mgmt/face-manager-command-fixture.hpp
index 0bde982..bf376f5 100644
--- a/tests/daemon/mgmt/face-manager-command-fixture.hpp
+++ b/tests/daemon/mgmt/face-manager-command-fixture.hpp
@@ -45,7 +45,14 @@
~FaceManagerCommandNode();
public:
- ndn::util::DummyClientFace face; ///< internal client face used by management
+ const Face*
+ findFaceByUri(const std::string& uri) const;
+
+ FaceId
+ findFaceIdByUri(const std::string& uri) const;
+
+public:
+ ndn::util::DummyClientFace face;
ndn::mgmt::Dispatcher dispatcher;
shared_ptr<CommandAuthenticator> authenticator;
diff --git a/tests/daemon/mgmt/face-manager-create-face.t.cpp b/tests/daemon/mgmt/face-manager-create-face.t.cpp
index ed89388..5cfe952 100644
--- a/tests/daemon/mgmt/face-manager-create-face.t.cpp
+++ b/tests/daemon/mgmt/face-manager-create-face.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -23,6 +23,7 @@
* NFD, e.g., in COPYING.md file. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "mgmt/face-manager.hpp"
#include "face-manager-command-fixture.hpp"
#include "nfd-manager-common-fixture.hpp"
@@ -206,8 +207,8 @@
ControlResponse actual(response.getContent().blockFromValue());
ControlResponse expected(CreateResult().getExpected());
- BOOST_CHECK_EQUAL(expected.getCode(), actual.getCode());
BOOST_TEST_MESSAGE(actual.getText());
+ BOOST_CHECK_EQUAL(expected.getCode(), actual.getCode());
if (actual.getBody().hasWire()) {
ControlParameters expectedParams(FaceType().getParameters());
@@ -249,25 +250,16 @@
BOOST_CHECK(hasCallbackFired);
}
-
-typedef mpl::vector<// mpl::pair<mpl::pair<TcpFacePersistent, TcpFacePermanent>, TcpFacePermanent>, // no need to check now
- // mpl::pair<mpl::pair<TcpFacePermanent, TcpFacePersistent>, TcpFacePersistent>, // no need to check now
- mpl::pair<mpl::pair<UdpFacePersistent, UdpFacePermanent>, UdpFacePermanent>,
- mpl::pair<mpl::pair<UdpFacePermanent, UdpFacePersistent>, UdpFacePermanent>> FaceTransitions;
-
-
-BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFace, T, FaceTransitions, FaceManagerCommandFixture)
+BOOST_FIXTURE_TEST_CASE(ExistingFace, FaceManagerCommandFixture)
{
- typedef typename T::first::first FaceType1;
- typedef typename T::first::second FaceType2;
- typedef typename T::second FinalFaceType;
+ using FaceType = UdpFacePersistent;
{
// create face
Name commandName("/localhost/nfd/faces");
commandName.append("create");
- commandName.append(FaceType1().getParameters().wireEncode());
+ commandName.append(FaceType().getParameters().wireEncode());
auto command = makeInterest(commandName);
m_keyChain.sign(*command);
@@ -275,116 +267,12 @@
this->advanceClocks(time::milliseconds(1), 5);
}
- //
- {
- // re-create face (= change face persistency)
-
- Name commandName("/localhost/nfd/faces");
- commandName.append("create");
- commandName.append(FaceType2().getParameters().wireEncode());
- auto command = makeInterest(commandName);
- m_keyChain.sign(*command);
-
- bool hasCallbackFired = false;
- this->node1.face.onSendData.connect([this, command, &hasCallbackFired] (const Data& response) {
- if (!command->getName().isPrefixOf(response.getName())) {
- return;
- }
-
- ControlResponse actual(response.getContent().blockFromValue());
- BOOST_REQUIRE_EQUAL(actual.getCode(), 200);
- BOOST_TEST_MESSAGE(actual.getText());
-
- ControlParameters expectedParams(FinalFaceType().getParameters());
- ControlParameters actualParams(actual.getBody());
- BOOST_REQUIRE(!actualParams.hasUri()); // Uri parameter is only included on command failure
- BOOST_CHECK_EQUAL(expectedParams.getFacePersistency(), actualParams.getFacePersistency());
-
- hasCallbackFired = true;
- });
-
- this->node1.face.receive(*command);
- this->advanceClocks(time::milliseconds(1), 5);
-
- BOOST_CHECK(hasCallbackFired);
- }
-}
-
-
-class UdpFace
-{
-public:
- ControlParameters
- getParameters()
- {
- return ControlParameters()
- .setUri("udp4://127.0.0.1:16363")
- .setFacePersistency(ndn::nfd::FACE_PERSISTENCY_PERSISTENT);
- }
-};
-
-
-// Note that the transitions from on-demand TcpFace are intentionally not tested.
-// On-demand TcpFace has a remote endpoint with a randomized port number. Normal face
-// creation operations will not need to create a face toward a remote port not listened by
-// a channel.
-
-typedef mpl::vector<mpl::pair<UdpFace, UdpFacePersistent>,
- mpl::pair<UdpFace, UdpFacePermanent>> OnDemandFaceTransitions;
-
-// need a slightly different logic to test transitions from OnDemand state
-BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFaceOnDemand, T, OnDemandFaceTransitions, FaceManagerCommandFixture)
-{
- typedef typename T::first OtherNodeFace;
- typedef typename T::second FaceType;
+ // find the created face
+ auto foundFace = this->node1.findFaceByUri(FaceType().getParameters().getUri());
+ BOOST_REQUIRE(foundFace != nullptr);
{
- // create on-demand face
-
- Name commandName("/localhost/nfd/faces");
- commandName.append("create");
- commandName.append(OtherNodeFace().getParameters().wireEncode());
- auto command = makeInterest(commandName);
- m_keyChain.sign(*command);
-
- ndn::util::signal::ScopedConnection connection =
- this->node2.face.onSendData.connect([this, command] (const Data& response) {
- if (!command->getName().isPrefixOf(response.getName())) {
- return;
- }
-
- ControlResponse controlResponse(response.getContent().blockFromValue());
- BOOST_REQUIRE_EQUAL(controlResponse.getText(), "OK");
- BOOST_REQUIRE_EQUAL(controlResponse.getCode(), 200);
- BOOST_REQUIRE(!ControlParameters(controlResponse.getBody()).hasUri());
- uint64_t faceId = ControlParameters(controlResponse.getBody()).getFaceId();
- auto face = this->node2.faceTable.get(static_cast<FaceId>(faceId));
-
- // to force creation of on-demand face
- auto dummyInterest = make_shared<Interest>("/hello/world");
- face->sendInterest(*dummyInterest);
- });
-
- this->node2.face.receive(*command);
- this->advanceClocks(time::milliseconds(1), 5); // let node2 process command and send Interest
- std::this_thread::sleep_for(std::chrono::milliseconds(100)); // allow wallclock time for socket IO
- this->advanceClocks(time::milliseconds(1), 5); // let node1 accept Interest and create on-demand face
- }
-
- // make sure there is on-demand face
- FaceUri onDemandFaceUri(FaceType().getParameters().getUri());
- const Face* foundFace = nullptr;
- for (const Face& face : this->node1.faceTable) {
- if (face.getRemoteUri() == onDemandFaceUri) {
- foundFace = &face;
- break;
- }
- }
- BOOST_REQUIRE_MESSAGE(foundFace != nullptr, "on-demand face is not created");
-
- //
- {
- // re-create face (= change face persistency)
+ // re-create face
Name commandName("/localhost/nfd/faces");
commandName.append("create");
@@ -400,14 +288,12 @@
}
ControlResponse actual(response.getContent().blockFromValue());
- BOOST_REQUIRE_EQUAL(actual.getCode(), 200);
+ BOOST_REQUIRE_EQUAL(actual.getCode(), 409);
- ControlParameters expectedParams(FaceType().getParameters());
ControlParameters actualParams(actual.getBody());
- BOOST_REQUIRE(!actualParams.hasUri());
- BOOST_CHECK_EQUAL(actualParams.getFacePersistency(), expectedParams.getFacePersistency());
- BOOST_CHECK_EQUAL(actualParams.getFaceId(), foundFace->getId());
- BOOST_CHECK_EQUAL(foundFace->getPersistency(), expectedParams.getFacePersistency());
+ BOOST_CHECK_EQUAL(foundFace->getId(), actualParams.getFaceId());
+ BOOST_CHECK_EQUAL(foundFace->getRemoteUri().toString(), actualParams.getUri());
+ BOOST_CHECK_EQUAL(foundFace->getPersistency(), actualParams.getFacePersistency());
hasCallbackFired = true;
});
diff --git a/tests/daemon/mgmt/face-manager-update-face.t.cpp b/tests/daemon/mgmt/face-manager-update-face.t.cpp
index df1f97f..6757622 100644
--- a/tests/daemon/mgmt/face-manager-update-face.t.cpp
+++ b/tests/daemon/mgmt/face-manager-update-face.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -23,10 +23,15 @@
* NFD, e.g., in COPYING.md file. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "mgmt/face-manager.hpp"
+#include "face/generic-link-service.hpp"
#include "face-manager-command-fixture.hpp"
#include "nfd-manager-common-fixture.hpp"
+
#include <ndn-cxx/lp/tags.hpp>
+#include <thread>
+
namespace nfd {
namespace tests {
@@ -56,36 +61,54 @@
params.setFacePersistency(persistency);
params.setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, enableLocalFields);
+ createFace(params);
+ }
+
+ void
+ createFace(const ControlParameters& createParams,
+ bool isForOnDemandFace = false)
+ {
Name commandName("/localhost/nfd/faces/create");
- commandName.append(params.wireEncode());
+ commandName.append(createParams.wireEncode());
auto command = makeInterest(commandName);
m_keyChain.sign(*command);
+ // if this creation if for on-demand face then create it on node2
+ FaceManagerCommandNode& target = isForOnDemandFace ? this->node2 : this->node1;
+
bool hasCallbackFired = false;
- signal::ScopedConnection connection = this->node1.face.onSendData.connect(
- [this, command, &hasCallbackFired] (const Data& response) {
+ signal::ScopedConnection connection = target.face.onSendData.connect(
+ [&, command, isForOnDemandFace, this] (const Data& response) {
if (!command->getName().isPrefixOf(response.getName())) {
return;
}
ControlResponse create(response.getContent().blockFromValue());
BOOST_REQUIRE_EQUAL(create.getCode(), 200);
+ BOOST_REQUIRE(create.getBody().hasWire());
- if (create.getBody().hasWire()) {
- ControlParameters faceParams(create.getBody());
- BOOST_REQUIRE(faceParams.hasFaceId());
- this->faceId = faceParams.getFaceId();
- }
- else {
- BOOST_FAIL("Face creation failed");
- }
+ ControlParameters faceParams(create.getBody());
+ BOOST_REQUIRE(faceParams.hasFaceId());
+ this->faceId = faceParams.getFaceId();
hasCallbackFired = true;
+
+ if (isForOnDemandFace) {
+ auto face = target.faceTable.get(static_cast<FaceId>(this->faceId));
+
+ // to force creation of on-demand face
+ face->sendInterest(*make_shared<Interest>("/hello/world"));
+ }
});
- this->node1.face.receive(*command);
+ target.face.receive(*command);
this->advanceClocks(time::milliseconds(1), 5);
+ if (isForOnDemandFace) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(100)); // allow wallclock time for socket IO
+ this->advanceClocks(time::milliseconds(1), 5); // let node1 accept Interest and create on-demand face
+ }
+
BOOST_REQUIRE(hasCallbackFired);
}
@@ -176,29 +199,63 @@
});
}
-// TODO #3232: Expected failure until FacePersistency updating implemented
-BOOST_AUTO_TEST_CASE(UpdatePersistency)
+template<bool CAN_CHANGE_PERSISTENCY>
+class UpdatePersistencyDummyTransport : public face::Transport
{
- createFace();
+public:
+ UpdatePersistencyDummyTransport()
+ {
+ this->setPersistency(ndn::nfd::FACE_PERSISTENCY_PERSISTENT);
+ }
- ControlParameters requestParams;
- requestParams.setFaceId(faceId);
- requestParams.setFacePersistency(ndn::nfd::FACE_PERSISTENCY_PERMANENT);
+protected:
+ bool
+ canChangePersistencyToImpl(ndn::nfd::FacePersistency newPersistency) const override
+ {
+ return CAN_CHANGE_PERSISTENCY;
+ }
- updateFace(requestParams, false, [] (const ControlResponse& actual) {
- ControlResponse expected(409, "Invalid fields specified");
- BOOST_CHECK_EQUAL(actual.getCode(), expected.getCode());
- BOOST_TEST_MESSAGE(actual.getText());
+ void
+ doClose() override
+ {
+ }
- if (actual.getBody().hasWire()) {
- ControlParameters actualParams(actual.getBody());
+private:
+ void
+ doSend(face::Transport::Packet&& packet) override
+ {
+ }
+};
- BOOST_REQUIRE(actualParams.hasFacePersistency());
- BOOST_CHECK_EQUAL(actualParams.getFacePersistency(), ndn::nfd::FACE_PERSISTENCY_PERMANENT);
- }
- else {
- BOOST_ERROR("Response does not contain ControlParameters");
- }
+namespace mpl = boost::mpl;
+
+using UpdatePersistencyTests =
+ mpl::vector<mpl::pair<UpdatePersistencyDummyTransport<true>, CommandSuccess>,
+ mpl::pair<UpdatePersistencyDummyTransport<false>, CommandFailure<409>>>;
+
+BOOST_FIXTURE_TEST_CASE_TEMPLATE(UpdatePersistency, T, UpdatePersistencyTests, FaceManagerUpdateFixture)
+{
+ using TransportType = typename T::first;
+ using ResultType = typename T::second;
+
+ auto face = make_shared<face::Face>(make_unique<face::GenericLinkService>(),
+ make_unique<TransportType>());
+ this->node1.faceTable.add(face);
+
+ auto parameters = ControlParameters()
+ .setFaceId(face->getId())
+ .setFacePersistency(ndn::nfd::FACE_PERSISTENCY_PERMANENT);
+
+ updateFace(parameters, false, [] (const ControlResponse& actual) {
+ BOOST_TEST_MESSAGE(actual.getText());
+ BOOST_CHECK_EQUAL(actual.getCode(), ResultType::getExpected().getCode());
+
+ // the response for either 200 or 409 will have a content body
+ BOOST_REQUIRE(actual.getBody().hasWire());
+
+ ControlParameters resp;
+ resp.wireDecode(actual.getBody());
+ BOOST_CHECK_EQUAL(resp.getFacePersistency(), ndn::nfd::FACE_PERSISTENCY_PERMANENT);
});
}
@@ -413,8 +470,6 @@
}
};
-namespace mpl = boost::mpl;
-
typedef mpl::vector<mpl::pair<TcpLocalFieldsEnable, CommandSuccess>,
mpl::pair<TcpLocalFieldsDisable, CommandSuccess>,
mpl::pair<UdpLocalFieldsEnable, CommandFailure<409>>,
diff --git a/tests/tools/nfdc/face-module.t.cpp b/tests/tools/nfdc/face-module.t.cpp
index 81ea707..5b44a5b 100644
--- a/tests/tools/nfdc/face-module.t.cpp
+++ b/tests/tools/nfdc/face-module.t.cpp
@@ -105,9 +105,25 @@
BOOST_AUTO_TEST_SUITE_END() // ShowCommand
-BOOST_FIXTURE_TEST_SUITE(CreateCommand, ExecuteCommandFixture)
+class ExecuteFaceCreateCommandFixture : public ExecuteCommandFixture
+{
+protected:
+ void
+ respond409(FacePersistency persistency)
+ {
+ MOCK_NFD_MGMT_REQUIRE_LAST_COMMAND_IS("/localhost/nfd/faces/create");
+ ControlParameters body;
+ body.setFaceId(1172)
+ .setUri("udp4://100.77.30.65:6363")
+ .setFacePersistency(persistency)
+ .setFlags(0);
+ this->failCommand(409, "conflict-409", body);
+ }
+};
-BOOST_AUTO_TEST_CASE(Normal)
+BOOST_FIXTURE_TEST_SUITE(CreateCommand, ExecuteFaceCreateCommandFixture)
+
+BOOST_AUTO_TEST_CASE(Creating)
{
this->processInterest = [this] (const Interest& interest) {
ControlParameters req = MOCK_NFD_MGMT_REQUIRE_LAST_COMMAND_IS("/localhost/nfd/faces/create");
@@ -128,7 +144,64 @@
BOOST_CHECK(err.is_empty());
}
-BOOST_AUTO_TEST_CASE(Error)
+BOOST_AUTO_TEST_CASE(UpgradingPersistency)
+{
+ bool hasUpdateCommand = false;
+ this->processInterest = [this, &hasUpdateCommand] (const Interest& interest) {
+ if (this->getCommand("/localhost/nfd/faces/create")) {
+ this->respond409(FacePersistency::FACE_PERSISTENCY_ON_DEMAND);
+ return;
+ }
+
+ ControlParameters req = MOCK_NFD_MGMT_REQUIRE_LAST_COMMAND_IS("/localhost/nfd/faces/update");
+ hasUpdateCommand = true;
+ BOOST_REQUIRE(req.hasFaceId());
+ BOOST_CHECK_EQUAL(req.getFaceId(), 1172);
+ BOOST_REQUIRE(req.hasFacePersistency());
+ BOOST_CHECK_EQUAL(req.getFacePersistency(), FacePersistency::FACE_PERSISTENCY_PERSISTENT);
+ BOOST_CHECK(!req.hasFlags());
+
+ ControlParameters resp;
+ resp.setFaceId(1172)
+ .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+ .setFlags(0);
+ this->succeedCommand(resp);
+ };
+
+ this->execute("face create udp://100.77.30.65");
+ BOOST_CHECK(hasUpdateCommand);
+ BOOST_CHECK_EQUAL(exitCode, 0);
+ BOOST_CHECK(out.is_equal("face-updated id=1172 remote=udp4://100.77.30.65:6363 persistency=persistent\n"));
+ BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(NotDowngradingPersistency)
+{
+ this->processInterest = [this] (const Interest& interest) {
+ this->respond409(FacePersistency::FACE_PERSISTENCY_PERMANENT);
+ // no command other than faces/create is expected
+ };
+
+ this->execute("face create udp://100.77.30.65");
+ BOOST_CHECK_EQUAL(exitCode, 0);
+ BOOST_CHECK(out.is_equal("face-exists id=1172 remote=udp4://100.77.30.65:6363 persistency=permanent\n"));
+ BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(SamePersistency)
+{
+ this->processInterest = [this] (const Interest& interest) {
+ this->respond409(FacePersistency::FACE_PERSISTENCY_PERSISTENT);
+ // no command other than faces/create is expected
+ };
+
+ this->execute("face create udp://100.77.30.65");
+ BOOST_CHECK_EQUAL(exitCode, 0);
+ BOOST_CHECK(out.is_equal("face-exists id=1172 remote=udp4://100.77.30.65:6363 persistency=persistent\n"));
+ BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(ErrorCreate)
{
this->processInterest = nullptr; // no response
@@ -138,6 +211,40 @@
BOOST_CHECK(err.is_equal("Error 10060 when creating face: request timed out\n"));
}
+BOOST_AUTO_TEST_CASE(ErrorConflict)
+{
+ // Current NFD will not report a 409-conflict with a different remote FaceUri, but this is
+ // allowed by FaceMgmt protocol and nfdc should not attempt to upgrade persistency in this case.
+
+ this->processInterest = [this] (const Interest& interest) {
+ // conflict with udp4://100.77.30.65:6363
+ this->respond409(FacePersistency::FACE_PERSISTENCY_ON_DEMAND);
+ };
+
+ this->execute("face create udp://20.53.73.45");
+ BOOST_CHECK_EQUAL(exitCode, 1);
+ BOOST_CHECK(out.is_empty());
+ BOOST_CHECK(err.is_equal("Error 409 when creating face: conflict-409\n"));
+}
+
+BOOST_AUTO_TEST_CASE(ErrorUpdate)
+{
+ this->processInterest = [this] (const Interest& interest) {
+ if (this->getCommand("/localhost/nfd/faces/create")) {
+ this->respond409(FacePersistency::FACE_PERSISTENCY_ON_DEMAND);
+ return;
+ }
+
+ MOCK_NFD_MGMT_REQUIRE_LAST_COMMAND_IS("/localhost/nfd/faces/update");
+ // no response to faces/update
+ };
+
+ this->execute("face create udp://100.77.30.65");
+ BOOST_CHECK_EQUAL(exitCode, 1);
+ BOOST_CHECK(out.is_empty());
+ BOOST_CHECK(err.is_equal("Error 10060 when upgrading face persistency: request timed out\n"));
+}
+
BOOST_AUTO_TEST_SUITE_END() // CreateCommand
BOOST_FIXTURE_TEST_SUITE(DestroyCommand, ExecuteCommandFixture)
diff --git a/tests/tools/nfdc/mock-nfd-mgmt-fixture.hpp b/tests/tools/nfdc/mock-nfd-mgmt-fixture.hpp
index 4d55849..485b030 100644
--- a/tests/tools/nfdc/mock-nfd-mgmt-fixture.hpp
+++ b/tests/tools/nfdc/mock-nfd-mgmt-fixture.hpp
@@ -63,7 +63,7 @@
* \return command parameters
*/
ndn::optional<ControlParameters>
- getCommand(const Name& expectedPrefix)
+ getCommand(const Name& expectedPrefix) const
{
if (face.sentInterests.empty() ||
!expectedPrefix.isPrefixOf(face.sentInterests.back().getName())) {
@@ -93,6 +93,12 @@
this->sendCommandReply({code, text});
}
+ void
+ failCommand(uint32_t code, const std::string& text, const ControlParameters& resp)
+ {
+ this->sendCommandReply(ndn::nfd::ControlResponse(code, text).setBody(resp.wireEncode()));
+ }
+
protected: // StatusDataset
/** \brief send an empty dataset in reply to StatusDataset request
* \param prefix dataset prefix without version and segment
diff --git a/tools/nfdc/execute-command.cpp b/tools/nfdc/execute-command.cpp
index 4dd4f24..92f1631 100644
--- a/tools/nfdc/execute-command.cpp
+++ b/tools/nfdc/execute-command.cpp
@@ -45,7 +45,7 @@
Controller::CommandFailCallback
ExecuteContext::makeCommandFailureHandler(const std::string& commandName)
{
- return [=] (const ndn::nfd::ControlResponse& resp) {
+ return [=] (const ControlResponse& resp) {
this->exitCode = 1;
this->err << "Error " << resp.getCode() << " when " << commandName << ": " << resp.getText() << '\n';
};
diff --git a/tools/nfdc/execute-command.hpp b/tools/nfdc/execute-command.hpp
index 09f28d5..2d8a716 100644
--- a/tools/nfdc/execute-command.hpp
+++ b/tools/nfdc/execute-command.hpp
@@ -43,6 +43,7 @@
using ndn::Face;
using ndn::KeyChain;
using ndn::nfd::ControlParameters;
+using ndn::nfd::ControlResponse;
using ndn::nfd::Controller;
/** \brief context for command execution
diff --git a/tools/nfdc/face-module.cpp b/tools/nfdc/face-module.cpp
index 982af63..3608ac2 100644
--- a/tools/nfdc/face-module.cpp
+++ b/tools/nfdc/face-module.cpp
@@ -77,25 +77,76 @@
}
}
+/** \brief order persistency in NONE < ON_DEMAND < PERSISTENCY < PERMANENT
+ */
+static bool
+persistencyLessThan(FacePersistency x, FacePersistency y)
+{
+ switch (x) {
+ case FacePersistency::FACE_PERSISTENCY_NONE:
+ return y != FacePersistency::FACE_PERSISTENCY_NONE;
+ case FacePersistency::FACE_PERSISTENCY_ON_DEMAND:
+ return y == FacePersistency::FACE_PERSISTENCY_PERSISTENT ||
+ y == FacePersistency::FACE_PERSISTENCY_PERMANENT;
+ case FacePersistency::FACE_PERSISTENCY_PERSISTENT:
+ return y == FacePersistency::FACE_PERSISTENCY_PERMANENT;
+ case FacePersistency::FACE_PERSISTENCY_PERMANENT:
+ return false;
+ }
+ return static_cast<int>(x) < static_cast<int>(y);
+}
+
void
FaceModule::create(ExecuteContext& ctx)
{
auto faceUri = ctx.args.get<FaceUri>("remote");
auto persistency = ctx.args.get<FacePersistency>("persistency", FacePersistency::FACE_PERSISTENCY_PERSISTENT);
+ FaceUri canonicalUri;
+
+ auto printPositiveResult = [&] (const std::string& actionSummary, const ControlParameters& resp) {
+ text::ItemAttributes ia;
+ ctx.out << actionSummary << ' '
+ << ia("id") << resp.getFaceId()
+ << ia("remote") << canonicalUri
+ << ia("persistency") << resp.getFacePersistency()
+ << '\n';
+ ///\todo #3956 display local=localUri before 'remote' field
+ };
+
+ auto handle409 = [&] (const ControlResponse& resp) {
+ ControlParameters respParams(resp.getBody());
+ if (respParams.getUri() != canonicalUri.toString()) {
+ // we are conflicting with a different face, which is a general error
+ return false;
+ }
+
+ if (persistencyLessThan(respParams.getFacePersistency(), persistency)) {
+ // need to upgrade persistency
+ ctx.controller.start<ndn::nfd::FaceUpdateCommand>(
+ ControlParameters().setFaceId(respParams.getFaceId()).setFacePersistency(persistency),
+ bind(printPositiveResult, "face-updated", _1),
+ ctx.makeCommandFailureHandler("upgrading face persistency"),
+ ctx.makeCommandOptions());
+ }
+ else {
+ // don't downgrade persistency
+ printPositiveResult("face-exists", respParams);
+ }
+ return true;
+ };
faceUri.canonize(
- [&] (const FaceUri& canonicalUri) {
+ [&] (const FaceUri& canonicalUri1) {
+ canonicalUri = canonicalUri1;
ctx.controller.start<ndn::nfd::FaceCreateCommand>(
ControlParameters().setUri(canonicalUri.toString()).setFacePersistency(persistency),
- [&ctx, canonicalUri] (const ControlParameters& resp) {
- ctx.out << "face-created ";
- text::ItemAttributes ia;
- ctx.out << ia("id") << resp.getFaceId()
- << ia("remote") << canonicalUri
- << ia("persistency") << resp.getFacePersistency() << '\n';
- ///\todo #3956 display local=localUri before 'remote' field
+ bind(printPositiveResult, "face-created", _1),
+ [&] (const ControlResponse& resp) {
+ if (resp.getCode() == 409 && handle409(resp)) {
+ return;
+ }
+ ctx.makeCommandFailureHandler("creating face")(resp); // invoke general error handler
},
- ctx.makeCommandFailureHandler("creating face"), ///\todo #3232 upgrade persistency if necessary upon 409
ctx.makeCommandOptions());
},
[&] (const std::string& canonizeError) {