Mailing-List: contact commits-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mesos.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: yan@apache.org
To: commits@mesos.apache.org
Date: Wed, 18 Jan 2017 01:47:07 -0000
Message-Id: <04ca2a8f7c3e4893898041638be2737f@git.apache.org>
In-Reply-To: <ddba7c54f03c412fbfadad33eb2fa4f9@git.apache.org>
References: <ddba7c54f03c412fbfadad33eb2fa4f9@git.apache.org>
Subject: [4/4] mesos git commit: Implemented TODOs to use common ID validation.
archived-at: Wed, 18 Jan 2017 01:47:06 -0000

Implemented TODOs to use common ID validation.

Review: https://reviews.apache.org/r/55448


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/2d21d96f
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/2d21d96f
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/2d21d96f

Branch: refs/heads/master
Commit: 2d21d96f701b3921b840176ccc405cacc984c329
Parents: e2d9ff0
Author: Jiang Yan Xu <xujyan@apple.com>
Authored: Mon Jan 9 15:48:33 2017 -0800
Committer: Jiang Yan Xu <xujyan@apple.com>
Committed: Tue Jan 17 17:38:38 2017 -0800

----------------------------------------------------------------------
 src/master/validation.cpp | 39 ++++++++++++---------------------------
 src/slave/validation.cpp  | 33 +++++++++++++--------------------
 2 files changed, 25 insertions(+), 47 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/2d21d96f/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 2c7e4ee..5f134b7 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -14,7 +14,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <algorithm>
+#include "master/validation.hpp"
+
 #include <string>
 #include <vector>
 
@@ -31,11 +32,11 @@
 #include <stout/stringify.hpp>
 
 #include "common/protobuf_utils.hpp"
+#include "common/validation.hpp"
 
 #include "health-check/health_checker.hpp"
 
 #include "master/master.hpp"
-#include "master/validation.hpp"
 
 using std::string;
 using std::vector;
@@ -46,15 +47,6 @@ namespace mesos {
 namespace internal {
 namespace master {
 namespace validation {
-
-// A helper function which returns true if the given character is not
-// suitable for an ID.
-static bool invalidCharacter(char c)
-{
-  return iscntrl(c) || c == '/' || c == '\\';
-}
-
-
 namespace master {
 namespace call {
 
@@ -506,12 +498,12 @@ Option<Error> validateDiskInfo(const RepeatedPtrField<Resource>& resources)
         return Error("Expecting 'host_path' to be unset for persistent volume");
       }
 
-      // Ensure persistence ID does not have invalid characters.
-      //
-      // TODO(bmahler): Validate against empty id!
-      string id = resource.disk().persistence().id();
-      if (std::any_of(id.begin(), id.end(), invalidCharacter)) {
-        return Error("Persistence ID '" + id + "' contains invalid characters");
+      // Ensure persistence ID meets common ID requirements.
+      Option<Error> error =
+        common::validation::validateID(resource.disk().persistence().id());
+      if (error.isSome()) {
+        return Error("Invalid persistence ID for persistent volume: " +
+                     error->message);
       }
     } else if (resource.disk().has_volume()) {
       return Error("Non-persistent volume not supported");
@@ -787,18 +779,11 @@ namespace task {
 
 namespace internal {
 
-// Validates that a task id is valid, i.e., contains only valid
-// characters.
 Option<Error> validateTaskID(const TaskInfo& task)
 {
-  const string& id = task.task_id().value();
-
-  // TODO(bmahler): Validate against empty id!
-  if (std::any_of(id.begin(), id.end(), invalidCharacter)) {
-    return Error("TaskID '" + id + "' contains invalid characters");
-  }
-
-  return None();
+  // Delegate to the common TaskID validation. Here we wrap
+  // around it to be consistent with other task validators.
+  return common::validation::validateTaskID(task.task_id());
 }
 
 
http://git-wip-us.apache.org/repos/asf/mesos/blob/2d21d96f/src/slave/validation.cpp
----------------------------------------------------------------------
diff --git a/src/slave/validation.cpp b/src/slave/validation.cpp
index da502a9..7f1ba93 100644
--- a/src/slave/validation.cpp
+++ b/src/slave/validation.cpp
@@ -14,7 +14,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <cctype>
+#include "slave/validation.hpp"
+
 #include <string>
 
 #include <mesos/agent/agent.hpp>
@@ -23,9 +24,7 @@
 #include <stout/unreachable.hpp>
 #include <stout/uuid.hpp>
 
-#include <stout/os/constants.hpp>
-
-#include "slave/validation.hpp"
+#include "common/validation.hpp"
 
 using std::string;
 
@@ -38,7 +37,15 @@ namespace container {
 
 Option<Error> validateContainerId(const ContainerID& containerId)
 {
-  // Slashes are disallowed as these IDs are mapped to directories.
+  const string& id = containerId.value();
+
+  // Check common Mesos ID rules.
+  Option<Error> error = common::validation::validateID(id);
+  if (error.isSome()) {
+    return Error(error->message);
+  }
+
+  // Check ContainerID specific rules.
   //
   // Periods are disallowed because our string representation of
   // ContainerID uses periods: <uuid>.<child>.<grandchild>.
@@ -46,24 +53,10 @@ Option<Error> validateContainerId(const ContainerID& containerId)
   //
   // Spaces are disallowed as they can render logs confusing and
   // need escaping on terminals when dealing with paths.
-  //
-  // TODO(bmahler): Add common/validation.hpp to share ID validation.
-  // Note that this however is slightly stricter than other IDs in
-  // that we do not allow periods or spaces.
   auto invalidCharacter = [](char c) {
-    return iscntrl(c) ||
-      c == os::POSIX_PATH_SEPARATOR ||
-      c == os::WINDOWS_PATH_SEPARATOR ||
-      c == '.' ||
-      c == ' ';
+    return  c == '.' || c == ' ';
   };
 
-  const string& id = containerId.value();
-
-  if (id.empty()) {
-    return Error("'ContainerID.value' must be non-empty");
-  }
-
   if (std::any_of(id.begin(), id.end(), invalidCharacter)) {
     return Error("'ContainerID.value' '" + id + "'"
                  " contains invalid characters");