mgmt+tools: allow MTU overrides on existing faces

refs #5056

Change-Id: I464a0c68773290bf1cfd0bf2afc4344e79fbb614
diff --git a/daemon/mgmt/face-manager.cpp b/daemon/mgmt/face-manager.cpp
index fd5f3aa..a6ff3df 100644
--- a/daemon/mgmt/face-manager.cpp
+++ b/daemon/mgmt/face-manager.cpp
@@ -117,7 +117,7 @@
     faceParams.defaultCongestionThreshold = parameters.getDefaultCongestionThreshold();
   }
   if (parameters.hasMtu()) {
-    // Cap this value at the maximum representable value in an ssize_t
+    // The face system limits MTUs to ssize_t, but the management protocol uses uint64_t
     faceParams.mtu = std::min<uint64_t>(std::numeric_limits<ssize_t>::max(), parameters.getMtu());
   }
   faceParams.wantLocalFields = parameters.hasFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED) &&
@@ -167,6 +167,7 @@
   ControlParameters params;
   params.setFaceId(face.getId())
         .setFacePersistency(face.getPersistency());
+  copyMtu(face, params);
 
   auto linkService = dynamic_cast<face::GenericLinkService*>(face.getLinkService());
   if (linkService != nullptr) {
@@ -188,8 +189,6 @@
   params.setUri(face.getRemoteUri().toString())
         .setLocalUri(face.getLocalUri().toString());
 
-  copyMtu(face, params);
-
   return params;
 }
 
@@ -244,6 +243,11 @@
     options.defaultCongestionThreshold = parameters.getDefaultCongestionThreshold();
   }
 
+  if (parameters.hasMtu()) {
+    // The face system limits MTUs to ssize_t, but the management protocol uses uint64_t
+    options.overrideMtu = std::min<uint64_t>(std::numeric_limits<ssize_t>::max(), parameters.getMtu());
+  }
+
   linkService->setOptions(options);
 }
 
@@ -293,6 +297,19 @@
     }
   }
 
+  // check whether the requested MTU override is valid (if it's present)
+  if (parameters.hasMtu()) {
+    auto mtu = parameters.getMtu();
+    // The face system limits MTUs to ssize_t, but the management protocol uses uint64_t
+    auto actualMtu = std::min<uint64_t>(std::numeric_limits<ssize_t>::max(), mtu);
+    auto linkService = dynamic_cast<face::GenericLinkService*>(face->getLinkService());
+    if (linkService == nullptr || !linkService->canOverrideMtuTo(actualMtu)) {
+      NFD_LOG_TRACE("cannot override face MTU to " << mtu);
+      areParamsValid = false;
+      response.setMtu(mtu);
+    }
+  }
+
   if (!areParamsValid) {
     done(ControlResponse(409, "Invalid properties specified").setBody(response.wireEncode()));
     return;
diff --git a/docs/manpages/nfdc-face.rst b/docs/manpages/nfdc-face.rst
index c00a6ce..c09191b 100644
--- a/docs/manpages/nfdc-face.rst
+++ b/docs/manpages/nfdc-face.rst
@@ -27,6 +27,8 @@
 The **nfdc face show** command shows properties and statistics of one specific face.
 
 The **nfdc face create** command creates a UDP unicast, TCP, or Ethernet unicast face.
+If the face already exists, the specified arguments will be used to update its properties, if
+possible.
 Local FaceUri is required for creating Ethernet unicast faces; otherwise it must be omitted.
 The NDNLPv2 unicast reliability feature may be explicitly enabled by specifying **reliability on**
 or explicitly disabled by specifying **reliability off**.
@@ -38,11 +40,10 @@
 default on all other face types.
 Parameters for this feature can set with the **congestion-marking-interval** option (specified in
 milliseconds) and the **default-congestion-threshold** option (specified in bytes).
-The MTUs of unicast Ethernet and UDP faces may be overridden using the **mtu** parameter (specified
-in bytes).
-However, a face may choose to limit the range of acceptable values for this parameter or disregard
-it altogether when setting its MTU.
-The MTU of an existing face may not be modified using this parameter and will result in an error.
+The effective MTUs of unicast Ethernet and UDP faces may be overridden using the **mtu** parameter
+(specified in bytes).
+The forwarder may limit the range of this override MTU and will use the minimum of it and the MTU
+of the underlying Ethernet or UDP transport.
 
 The **nfdc face destroy** command destroys an existing face.
 
@@ -93,6 +94,12 @@
     It is the maximum bound of the congestion threshold for the face, as well as the default
     threshold used if the face does not support retrieving the capacity of the send queue.
 
+<MTU>
+    The MTU used to override the MTU of the underlying transport on Ethernet and UDP faces.
+    This MTU serves as an upper bound for the MTU provided by the transport.
+    The range of acceptable values may be limited by the forwarder.
+    To unset this override, specify the MTU as "auto".
+
 EXIT CODES
 ----------
 0: Success
@@ -135,8 +142,7 @@
     Create a face with the specified remote FaceUri and explicitly disable congestion marking.
 
 nfdc face create remote udp://router.example.net mtu 4000
-    Create a face with the specified remote FaceUri and an MTU of 4000 bytes (which may be ignored
-    or limited to within a certain range by the internal logic of the face).
+    Create a face with the specified remote FaceUri and set the override MTU to 4000 bytes.
 
 nfdc face destroy 300
     Destroy the face whose FaceId is 300.
diff --git a/tests/daemon/mgmt/face-manager-update-face.t.cpp b/tests/daemon/mgmt/face-manager-update-face.t.cpp
index f7e89f8..c2f0131 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-2019,  Regents of the University of California,
+ * Copyright (c) 2014-2020,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -54,8 +54,8 @@
   void
   createFace(const std::string& uri = "tcp4://127.0.0.1:26363",
              ndn::nfd::FacePersistency persistency = ndn::nfd::FACE_PERSISTENCY_PERSISTENT,
-             optional<time::nanoseconds> baseCongestionMarkingInterval = {},
-             optional<uint64_t> defaultCongestionThreshold = {},
+             optional<time::nanoseconds> baseCongestionMarkingInterval = nullopt,
+             optional<uint64_t> defaultCongestionThreshold = nullopt,
              bool enableLocalFields = false,
              bool enableReliability = false,
              boost::logic::tribool enableCongestionMarking = boost::logic::indeterminate)
@@ -235,6 +235,79 @@
   });
 }
 
+BOOST_AUTO_TEST_CASE(UpdateMtu)
+{
+  createFace("udp4://127.0.0.1:26363");
+
+  ControlParameters validParams;
+  validParams.setFaceId(faceId);
+  validParams.setMtu(4000);
+
+  ControlParameters mtuTooLow;
+  mtuTooLow.setFaceId(faceId);
+  mtuTooLow.setMtu(63);
+
+  updateFace(validParams, false, [] (const ControlResponse& actual) {
+    BOOST_CHECK_EQUAL(actual.getCode(), 200);
+    BOOST_TEST_MESSAGE(actual.getText());
+
+    if (actual.getBody().hasWire()) {
+      ControlParameters actualParams(actual.getBody());
+
+      BOOST_CHECK(actualParams.hasFaceId());
+      BOOST_REQUIRE(actualParams.hasMtu());
+      // Check for changed MTU
+      BOOST_CHECK_EQUAL(actualParams.getMtu(), 4000);
+    }
+    else {
+      BOOST_ERROR("Valid: Response does not contain ControlParameters");
+    }
+  });
+
+  updateFace(mtuTooLow, false, [] (const ControlResponse& actual) {
+    BOOST_CHECK_EQUAL(actual.getCode(), 409);
+    BOOST_TEST_MESSAGE(actual.getText());
+
+    if (actual.getBody().hasWire()) {
+      ControlParameters actualParams(actual.getBody());
+
+      BOOST_CHECK(!actualParams.hasFaceId());
+      BOOST_REQUIRE(actualParams.hasMtu());
+      // Check for returned invalid parameter
+      BOOST_CHECK_EQUAL(actualParams.getMtu(), 63);
+    }
+    else {
+      BOOST_ERROR("Too low: Response does not contain ControlParameters");
+    }
+  });
+}
+
+BOOST_AUTO_TEST_CASE(UpdateMtuUnsupportedFace)
+{
+  createFace("tcp4://127.0.0.1:26363");
+
+  ControlParameters updateParams;
+  updateParams.setFaceId(faceId);
+  updateParams.setMtu(4000);
+
+  updateFace(updateParams, false, [] (const ControlResponse& actual) {
+    BOOST_CHECK_EQUAL(actual.getCode(), 409);
+    BOOST_TEST_MESSAGE(actual.getText());
+
+    if (actual.getBody().hasWire()) {
+      ControlParameters actualParams(actual.getBody());
+
+      BOOST_CHECK(!actualParams.hasFaceId());
+      BOOST_REQUIRE(actualParams.hasMtu());
+      // Check for returned invalid parameter
+      BOOST_CHECK_EQUAL(actualParams.getMtu(), 4000);
+    }
+    else {
+      BOOST_ERROR("Response does not contain ControlParameters");
+    }
+  });
+}
+
 class TcpLocalFieldsEnable
 {
 public:
diff --git a/tests/tools/nfdc/face-module.t.cpp b/tests/tools/nfdc/face-module.t.cpp
index a3f4618..84031dd 100644
--- a/tests/tools/nfdc/face-module.t.cpp
+++ b/tests/tools/nfdc/face-module.t.cpp
@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2018,  Regents of the University of California,
+ * Copyright (c) 2014-2020,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -476,17 +476,74 @@
   BOOST_CHECK(err.is_empty());
 }
 
-BOOST_AUTO_TEST_CASE(MtuExistingFace)
+BOOST_AUTO_TEST_CASE(ChangingMtu)
 {
-  this->processInterest = [this] (const Interest& interest) {
-    this->respond409(interest, FacePersistency::FACE_PERSISTENCY_ON_DEMAND, 4000);
-    // no command other than faces/create is expected
+  bool hasUpdateCommand = false;
+  this->processInterest = [this, &hasUpdateCommand] (const Interest& interest) {
+    if (parseCommand(interest, "/localhost/nfd/faces/create")) {
+      this->respond409(interest, FacePersistency::FACE_PERSISTENCY_PERSISTENT, 5000);
+      return;
+    }
+
+    ControlParameters req = MOCK_NFD_MGMT_REQUIRE_COMMAND_IS("/localhost/nfd/faces/update");
+    hasUpdateCommand = true;
+    BOOST_REQUIRE(req.hasFaceId());
+    BOOST_CHECK_EQUAL(req.getFaceId(), 1172);
+    BOOST_CHECK(!req.hasFacePersistency());
+    BOOST_REQUIRE(req.hasMtu());
+    BOOST_CHECK_EQUAL(req.getMtu(), 4000);
+    BOOST_CHECK(!req.hasFlags());
+
+    ControlParameters resp;
+    resp.setFaceId(1172)
+        .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(4000)
+        .setFlags(0);
+    this->succeedCommand(interest, resp);
   };
 
-  this->execute("face create udp://100.77.30.65 mtu 5000");
-  BOOST_CHECK_EQUAL(exitCode, 1);
-  BOOST_CHECK(out.is_empty());
-  BOOST_CHECK(err.is_equal("Error 409 when creating face: conflict-409\n"));
+  this->execute("face create udp://100.77.30.65 mtu 4000");
+  BOOST_CHECK(hasUpdateCommand);
+  BOOST_CHECK_EQUAL(exitCode, 0);
+  BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
+                           "remote=udp4://100.77.30.65:6363 persistency=persistent "
+                           "reliability=off congestion-marking=off mtu=4000\n"));
+  BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(AutoMtu)
+{
+  bool hasUpdateCommand = false;
+  this->processInterest = [this, &hasUpdateCommand] (const Interest& interest) {
+    if (parseCommand(interest, "/localhost/nfd/faces/create")) {
+      this->respond409(interest, FacePersistency::FACE_PERSISTENCY_PERSISTENT, 5000);
+      return;
+    }
+
+    ControlParameters req = MOCK_NFD_MGMT_REQUIRE_COMMAND_IS("/localhost/nfd/faces/update");
+    hasUpdateCommand = true;
+    BOOST_REQUIRE(req.hasFaceId());
+    BOOST_CHECK_EQUAL(req.getFaceId(), 1172);
+    BOOST_CHECK(!req.hasFacePersistency());
+    BOOST_REQUIRE(req.hasMtu());
+    BOOST_CHECK_EQUAL(req.getMtu(), std::numeric_limits<uint64_t>::max());
+    BOOST_CHECK(!req.hasFlags());
+
+    ControlParameters resp;
+    resp.setFaceId(1172)
+        .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(ndn::MAX_NDN_PACKET_SIZE)
+        .setFlags(0);
+    this->succeedCommand(interest, resp);
+  };
+
+  this->execute("face create udp://100.77.30.65 mtu auto");
+  BOOST_CHECK(hasUpdateCommand);
+  BOOST_CHECK_EQUAL(exitCode, 0);
+  BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
+                           "remote=udp4://100.77.30.65:6363 persistency=persistent "
+                           "reliability=off congestion-marking=off mtu=8800\n"));
+  BOOST_CHECK(err.is_empty());
 }
 
 BOOST_AUTO_TEST_CASE(UpgradingPersistency)
@@ -509,6 +566,7 @@
     ControlParameters resp;
     resp.setFaceId(1172)
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(1024)
         .setFlags(0);
     this->succeedCommand(interest, resp);
   };
@@ -518,7 +576,7 @@
   BOOST_CHECK_EQUAL(exitCode, 0);
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
-                           "reliability=off congestion-marking=off\n"));
+                           "reliability=off congestion-marking=off mtu=1024\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -537,11 +595,13 @@
     BOOST_CHECK_EQUAL(req.getFaceId(), 1172);
     BOOST_REQUIRE(req.hasFacePersistency());
     BOOST_CHECK_EQUAL(req.getFacePersistency(), FacePersistency::FACE_PERSISTENCY_PERSISTENT);
+    BOOST_CHECK(!req.hasMtu());
     BOOST_CHECK(!req.hasFlags());
 
     ControlParameters resp;
     resp.setFaceId(1172)
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(8800)
         .setFlags(0);
     this->succeedCommand(interest, resp);
   };
@@ -551,7 +611,7 @@
   BOOST_CHECK_EQUAL(exitCode, 0);
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
-                           "reliability=off congestion-marking=off\n"));
+                           "reliability=off congestion-marking=off mtu=8800\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -604,6 +664,7 @@
     ControlParameters resp;
     resp.setFaceId(1172)
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(4000)
         .setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, true, false);
     this->succeedCommand(interest, resp);
   };
@@ -612,7 +673,7 @@
   BOOST_CHECK_EQUAL(exitCode, 0);
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
-                           "reliability=on congestion-marking=off\n"));
+                           "reliability=on congestion-marking=off mtu=4000\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -635,6 +696,7 @@
     ControlParameters resp;
     resp.setFaceId(1172)
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(4000)
         .setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, false, false);
     this->succeedCommand(interest, resp);
   };
@@ -643,7 +705,7 @@
   BOOST_CHECK_EQUAL(exitCode, 0);
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
-                           "reliability=off congestion-marking=off\n"));
+                           "reliability=off congestion-marking=off mtu=4000\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -670,6 +732,7 @@
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
         .setBaseCongestionMarkingInterval(100_ms)
         .setDefaultCongestionThreshold(65536)
+        .setMtu(4000)
         .setFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED, true, false);
     this->succeedCommand(interest, resp);
   };
@@ -679,7 +742,8 @@
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
                            "reliability=off congestion-marking=on "
-                           "congestion-marking-interval=100ms default-congestion-threshold=65536B\n"));
+                           "congestion-marking-interval=100ms default-congestion-threshold=65536B "
+                           "mtu=4000\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -706,6 +770,7 @@
         .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
         .setBaseCongestionMarkingInterval(100_ms)
         .setDefaultCongestionThreshold(65536)
+        .setMtu(4000)
         .setFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED, false, false);
     this->succeedCommand(interest, resp);
   };
@@ -715,7 +780,81 @@
   BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
                            "remote=udp4://100.77.30.65:6363 persistency=persistent "
                            "reliability=off congestion-marking=off "
-                           "congestion-marking-interval=100ms default-congestion-threshold=65536B\n"));
+                           "congestion-marking-interval=100ms default-congestion-threshold=65536B "
+                           "mtu=4000\n"));
+  BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(UpgradingPersistencyChangeMtu)
+{
+  bool hasUpdateCommand = false;
+  this->processInterest = [this, &hasUpdateCommand] (const Interest& interest) {
+    if (parseCommand(interest, "/localhost/nfd/faces/create")) {
+      this->respond409(interest, FacePersistency::FACE_PERSISTENCY_ON_DEMAND, 8800);
+      return;
+    }
+
+    ControlParameters req = MOCK_NFD_MGMT_REQUIRE_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_REQUIRE(req.hasMtu());
+    BOOST_CHECK_EQUAL(req.getMtu(), 4000);
+    BOOST_CHECK(!req.hasFlags());
+
+    ControlParameters resp;
+    resp.setFaceId(1172)
+        .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(4000)
+        .setFlags(0);
+    this->succeedCommand(interest, resp);
+  };
+
+  this->execute("face create udp://100.77.30.65 mtu 4000");
+  BOOST_CHECK(hasUpdateCommand);
+  BOOST_CHECK_EQUAL(exitCode, 0);
+  BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
+                           "remote=udp4://100.77.30.65:6363 persistency=persistent "
+                           "reliability=off congestion-marking=off mtu=4000\n"));
+  BOOST_CHECK(err.is_empty());
+}
+
+BOOST_AUTO_TEST_CASE(UpgradingPersistencyChangeMtuAndFlags)
+{
+  bool hasUpdateCommand = false;
+  this->processInterest = [this, &hasUpdateCommand] (const Interest& interest) {
+    if (parseCommand(interest, "/localhost/nfd/faces/create")) {
+      this->respond409(interest, FacePersistency::FACE_PERSISTENCY_ON_DEMAND, 8800);
+      return;
+    }
+
+    ControlParameters req = MOCK_NFD_MGMT_REQUIRE_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_REQUIRE(req.hasMtu());
+    BOOST_CHECK_EQUAL(req.getMtu(), 4000);
+    BOOST_CHECK(req.hasFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED));
+    BOOST_CHECK(req.getFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED));
+
+    ControlParameters resp;
+    resp.setFaceId(1172)
+        .setFacePersistency(FacePersistency::FACE_PERSISTENCY_PERSISTENT)
+        .setMtu(4000)
+        .setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, true, false);
+    this->succeedCommand(interest, resp);
+  };
+
+  this->execute("face create udp://100.77.30.65 mtu 4000 reliability on");
+  BOOST_CHECK(hasUpdateCommand);
+  BOOST_CHECK_EQUAL(exitCode, 0);
+  BOOST_CHECK(out.is_equal("face-updated id=1172 local=udp4://68.62.26.57:24087 "
+                           "remote=udp4://100.77.30.65:6363 persistency=persistent "
+                           "reliability=on congestion-marking=off mtu=4000\n"));
   BOOST_CHECK(err.is_empty());
 }
 
@@ -776,7 +915,7 @@
   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_CHECK(err.is_equal("Error 10060 when updating face: request timed out\n"));
 }
 
 BOOST_AUTO_TEST_SUITE_END() // CreateCommand
diff --git a/tools/nfdc/face-module.cpp b/tools/nfdc/face-module.cpp
index 83c6006..37f5bd9 100644
--- a/tools/nfdc/face-module.cpp
+++ b/tools/nfdc/face-module.cpp
@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2019,  Regents of the University of California,
+ * Copyright (c) 2014-2020,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -58,7 +58,7 @@
     .addArg("congestion-marking", ArgValueType::BOOLEAN, Required::NO, Positional::NO)
     .addArg("congestion-marking-interval", ArgValueType::UNSIGNED, Required::NO, Positional::NO)
     .addArg("default-congestion-threshold", ArgValueType::UNSIGNED, Required::NO, Positional::NO)
-    .addArg("mtu", ArgValueType::UNSIGNED, Required::NO, Positional::NO);
+    .addArg("mtu", ArgValueType::STRING, Required::NO, Positional::NO);
   parser.addCommand(defFaceCreate, &FaceModule::create);
 
   CommandDefinition defFaceDestroy("face", "destroy");
@@ -160,13 +160,30 @@
   auto congestionMarking = ctx.args.getTribool("congestion-marking");
   auto baseCongestionMarkingIntervalMs = ctx.args.getOptional<uint64_t>("congestion-marking-interval");
   auto defaultCongestionThreshold = ctx.args.getOptional<uint64_t>("default-congestion-threshold");
-  auto mtu = ctx.args.getOptional<uint64_t>("mtu");
+  auto mtuArg = ctx.args.getOptional<std::string>("mtu");
+
+  // MTU is nominally a uint64_t, but can be the string value 'auto' to unset an override MTU
+  optional<uint64_t> mtu;
+  if (mtuArg == "auto") {
+    mtu = std::numeric_limits<uint64_t>::max();
+  }
+  else if (mtuArg) {
+    // boost::lexical_cast<uint64_t> will accept negative numbers
+    int64_t v = -1;
+    if (!boost::conversion::try_lexical_convert<int64_t>(*mtuArg, v) || v < 0) {
+      ctx.exitCode = 2;
+      ctx.err << "MTU must either be a non-negative integer or 'auto'\n";
+      return;
+    }
+
+    mtu = static_cast<uint64_t>(v);
+  }
 
   FaceUri canonicalRemote;
   optional<FaceUri> canonicalLocal;
 
   auto handleCanonizeError = [&] (const FaceUri& faceUri, const std::string& error) {
-    ctx.exitCode = 4;
+    ctx.exitCode = static_cast<int>(FindFace::Code::CANONIZE_ERROR);
     ctx.err << "Error when canonizing '" << faceUri << "': " << error << '\n';
   };
 
@@ -194,59 +211,54 @@
       return false;
     }
 
+    bool isChangingParams = false;
+    ControlParameters params;
+    params.setFaceId(respParams.getFaceId());
+
     if (mtu && (!respParams.hasMtu() || respParams.getMtu() != *mtu)) {
-      // Mtu cannot be changed with faces/update
-      return false;
+      isChangingParams = true;
+      params.setMtu(*mtu);
     }
-    else if (persistencyLessThan(respParams.getFacePersistency(), persistency)) {
-      // need to upgrade persistency
-      ControlParameters params;
-      params.setFaceId(respParams.getFaceId()).setFacePersistency(persistency);
-      if (!boost::logic::indeterminate(lpReliability)) {
-        params.setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, bool(lpReliability));
-      }
-      ctx.controller.start<ndn::nfd::FaceUpdateCommand>(
-          params,
-          bind(updateFace, respParams, _1),
-          ctx.makeCommandFailureHandler("upgrading face persistency"),
-          ctx.makeCommandOptions());
+
+    if (persistencyLessThan(respParams.getFacePersistency(), persistency)) {
+      isChangingParams = true;
+      params.setFacePersistency(persistency);
     }
-    else if ((!boost::logic::indeterminate(lpReliability) &&
-              lpReliability != respParams.getFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED)) ||
-             (!boost::logic::indeterminate(congestionMarking) &&
-              congestionMarking != respParams.getFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED)) ||
-             baseCongestionMarkingIntervalMs ||
-             defaultCongestionThreshold) {
-      ControlParameters params;
-      params.setFaceId(respParams.getFaceId());
 
-      if (!boost::logic::indeterminate(lpReliability) &&
-          lpReliability != respParams.getFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED)) {
-        params.setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, bool(lpReliability));
-      }
-      if (!boost::logic::indeterminate(congestionMarking) &&
-          congestionMarking != respParams.getFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED)) {
-        params.setFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED, bool(congestionMarking));
-      }
+    if (!boost::logic::indeterminate(lpReliability) &&
+        lpReliability != respParams.getFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED)) {
+      isChangingParams = true;
+      params.setFlagBit(ndn::nfd::BIT_LP_RELIABILITY_ENABLED, bool(lpReliability));
+    }
 
-      if (baseCongestionMarkingIntervalMs) {
-        params.setBaseCongestionMarkingInterval(time::milliseconds(*baseCongestionMarkingIntervalMs));
-      }
+    if (!boost::logic::indeterminate(congestionMarking) &&
+        congestionMarking != respParams.getFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED)) {
+      isChangingParams = true;
+      params.setFlagBit(ndn::nfd::BIT_CONGESTION_MARKING_ENABLED, bool(congestionMarking));
+    }
 
-      if (defaultCongestionThreshold) {
-        params.setDefaultCongestionThreshold(*defaultCongestionThreshold);
-      }
+    if (baseCongestionMarkingIntervalMs) {
+      isChangingParams = true;
+      params.setBaseCongestionMarkingInterval(time::milliseconds(*baseCongestionMarkingIntervalMs));
+    }
 
+    if (defaultCongestionThreshold) {
+      isChangingParams = true;
+      params.setDefaultCongestionThreshold(*defaultCongestionThreshold);
+    }
+
+    if (isChangingParams) {
       ctx.controller.start<ndn::nfd::FaceUpdateCommand>(
-          params,
-          bind(updateFace, respParams, _1),
-          ctx.makeCommandFailureHandler("updating face"),
-          ctx.makeCommandOptions());
+        params,
+        bind(updateFace, respParams, _1),
+        ctx.makeCommandFailureHandler("updating face"),
+        ctx.makeCommandOptions());
     }
     else {
       // don't do anything
       printPositiveResult("face-exists", respParams);
     }
+
     return true;
   };