Correct parsing of ppr directives

Signed-off-by: Ralph Castain <rhc@open-mpi.org>

orte/mca/rmaps/base/rmaps_base_frame.c

@@ -12,7 +12,7 @@
  * Copyright (c) 2006-2015 Cisco Systems, Inc.  All rights reserved.
  * Copyright (c) 2011-2013 Los Alamos National Security, LLC.
  *                         All rights reserved.
- * Copyright (c) 2014-2018 Intel, Inc. All rights reserved.
+ * Copyright (c) 2014-2019 Intel, Inc.  All rights reserved.
  * Copyright (c) 2014-2015 Research Organization for Information Science
  *                         and Technology (RIST). All rights reserved.
  * $COPYRIGHT$
@@ -620,7 +620,9 @@ int orte_rmaps_base_set_mapping_policy(orte_job_t *jdata,
 
     if (NULL == inspec) {
         ORTE_SET_MAPPING_POLICY(tmp, ORTE_MAPPING_BYSOCKET);
-    } else {
+        goto setpolicy;
+    }
+
     spec = strdup(inspec);  // protect the input string
     /* see if a colon was included - if so, then we have a policy + modifier */
     ck = strchr(spec, ':');
@@ -628,7 +630,7 @@ int orte_rmaps_base_set_mapping_policy(orte_job_t *jdata,
         /* if the colon is the first character of the string, then we
          * just have modifiers on the default mapping policy */
         if (ck == spec) {
-                ck++;
+            ck++;  // step over the colon
             opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
                                 "%s rmaps:base only modifiers %s provided - assuming bysocket mapping",
                                 ORTE_NAME_PRINT(ORTE_PROC_MY_NAME), ck);
@@ -641,20 +643,20 @@ int orte_rmaps_base_set_mapping_policy(orte_job_t *jdata,
             free(spec);
             goto setpolicy;
         }
-            /* split the string */
-            *ck = '\0';
-            ck++;
+        *ck = '\0';  // terminate spec where the colon was
+        ck++;    // step past the colon
         opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
                             "%s rmaps:base policy %s modifiers %s provided",
                             ORTE_NAME_PRINT(ORTE_PROC_MY_NAME), spec, ck);
-            /* if the policy is "dist", then we set the policy to that value
-             * and save the second argument as the device
-             */
+
         if (0 == strncasecmp(spec, "ppr", strlen(spec))) {
-                /* we have to allow additional modifiers here - e.g., specifying
-                 * #pe's/proc or oversubscribe - so check for modifiers
+            /* at this point, ck points to a string that contains at least
+             * two fields (specifying the #procs/obj and the object we are
+             * to map by). we have to allow additional modifiers here - e.g.,
+             * specifying #pe's/proc or oversubscribe - so check for modifiers. if
+             * they are present, ck will look like "N:obj:mod1,mod2,mod3"
              */
-                if (NULL == (ptr = strrchr(ck, ':'))) {
+            if (NULL == (ptr = strchr(ck, ':'))) {
                 /* this is an error - there had to be at least one
                  * colon to delimit the number from the object type
                  */
@@ -663,15 +665,14 @@ int orte_rmaps_base_set_mapping_policy(orte_job_t *jdata,
                 return ORTE_ERR_SILENT;
             }
             ptr++; // move past the colon
-                /* at this point, ck is pointing to the number of procs/object
-                 * and ptr is pointing to the beginning of the string that describes
-                 * the object plus any modifiers. We first check to see if there
-                 * is a comma indicating that there are modifiers to the request */
-                if (NULL != (cptr = strchr(ptr, ','))) {
+            /* at this point, ptr is pointing to the beginning of the string that describes
+             * the object plus any modifiers (i.e., "obj:mod1,mod2". We first check to see if there
+             * is another colon indicating that there are modifiers to the request */
+            if (NULL != (cptr = strchr(ptr, ':'))) {
                 /* there are modifiers, so we terminate the object string
-                     * at the location of the first comma */
+                 * at the location of the colon */
                 *cptr = '\0';
-                    /* step over that comma */
+                /* step over that colon */
                 cptr++;
                 /* now check for modifiers  - may be none, so
                  * don't emit an error message if the modifier
@@ -750,7 +751,6 @@ int orte_rmaps_base_set_mapping_policy(orte_job_t *jdata,
     }
     free(spec);
     ORTE_SET_MAPPING_DIRECTIVE(tmp, ORTE_MAPPING_GIVEN);
-    }
 
  setpolicy:
     if (NULL == jdata || NULL == jdata->map) {