Skip to content

Commit

Permalink
Move the registration of MCA params out of the init of the var system…
Browse files Browse the repository at this point in the history 
… - put them in with the rest of the OPAL MCA param registrations

Take another shot at untangling the spaghetti

orterun: fix for command line parsing

orte-submit calls opal_init_util () before parsing out MCA command line
options (-mca, -am, etc). This prevents mpirun from setting opal MCA
variables for some frameworks as well as the MCA base. This is because
when a framework is opened all of its variables are set to read-only.
Eventually we want to lift this restriction on some MCA variables but
since -mca is affected we must parse out the MCA command line options
before opal_init_util(). This commit fixes the bug by adding a new
option to opal_cmd_line_parse (ignore unknown option) so orte-submit
can pre-parse the command line for MCA options.

Signed-off-by: Nathan Hjelm <[email protected]>

Minor cleanups to avoid releasing/recreating the cmd line
  • Loading branch information
Ralph Castain committed May 20, 2016
1 parent 5ec1eed commit 42ecffb
Show file tree
Hide file tree
Showing 21 changed files with 157 additions and 103 deletions.
2 changes: 1 addition & 1 deletion opal/mca/base/mca_base_parse_paramfile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "opal/mca/base/base.h"
#include "opal/mca/base/mca_base_vari.h"
#include "opal/util/keyval_parse.h"

#include "opal/util/output.h"
static void save_value(const char *name, const char *value);

static char * file_being_read;
Expand Down
59 changes: 15 additions & 44 deletions opal/mca/base/mca_base_var.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,9 @@ static char *mca_base_var_override_file = NULL;
static char *mca_base_var_file_prefix = NULL;
static char *mca_base_envar_file_prefix = NULL;
static char *mca_base_param_file_path = NULL;
static char *mca_base_env_list = NULL;
#define MCA_BASE_ENV_LIST_SEP_DEFAULT ";"
static char *mca_base_env_list_sep = MCA_BASE_ENV_LIST_SEP_DEFAULT;
static char *mca_base_env_list_internal = NULL;
char *mca_base_env_list = NULL;
char *mca_base_env_list_sep = MCA_BASE_ENV_LIST_SEP_DEFAULT;
char *mca_base_env_list_internal = NULL;
static bool mca_base_var_suppress_override_warning = false;
static opal_list_t mca_base_var_file_values;
static opal_list_t mca_base_envar_file_values;
Expand Down Expand Up @@ -130,7 +129,6 @@ static const char *info_lvl_strings[] = {
*/
static int fixup_files(char **file_list, char * path, bool rel_path_search, char sep);
static int read_files (char *file_list, opal_list_t *file_values, char sep);
static int mca_base_var_cache_files (bool rel_path_search);
static int var_set_initial (mca_base_var_t *var, mca_base_var_t *original);
static int var_get (int vari, mca_base_var_t **var_out, bool original);
static int var_value_string (mca_base_var_t *var, char **value_string);
Expand Down Expand Up @@ -242,7 +240,6 @@ static char *append_filename_to_list(const char *filename)
int mca_base_var_init(void)
{
int ret;
char *name = NULL;

if (!mca_base_var_initialized) {
/* Init the value array for the param storage */
Expand Down Expand Up @@ -282,41 +279,6 @@ int mca_base_var_init(void)

mca_base_var_initialized = true;

mca_base_var_cache_files(false);

/* register the envar-forwarding params */
(void)mca_base_var_register ("opal", "mca", "base", "env_list",
"Set SHELL env variables",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list);

mca_base_env_list_sep = MCA_BASE_ENV_LIST_SEP_DEFAULT;
(void)mca_base_var_register ("opal", "mca", "base", "env_list_delimiter",
"Set SHELL env variables delimiter. Default: semicolon ';'",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list_sep);

/* Set OMPI_MCA_mca_base_env_list variable, it might not be set before
* if mca variable was taken from amca conf file. Need to set it
* here because mca_base_var_process_env_list is called from schizo_ompi.c
* only when this env variable was set.
*/
if (NULL != mca_base_env_list) {
(void) mca_base_var_env_name ("mca_base_env_list", &name);
if (NULL != name) {
opal_setenv(name, mca_base_env_list, false, &environ);
free(name);
}
}

/* Register internal MCA variable mca_base_env_list_internal. It can be set only during
* parsing of amca conf file and contains SHELL env variables specified via -x there.
* Its format is the same as for mca_base_env_list.
*/
(void)mca_base_var_register ("opal", "mca", "base", "env_list_internal",
"Store SHELL env variables from amca conf file",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, MCA_BASE_VAR_FLAG_INTERNAL, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list_internal);
}

return OPAL_SUCCESS;
Expand Down Expand Up @@ -419,15 +381,15 @@ static void resolve_relative_paths(char **file_prefix, char *file_path, bool rel
}
}

static int mca_base_var_cache_files(bool rel_path_search)
int mca_base_var_cache_files(bool rel_path_search)
{
char *tmp;
int ret;

/* We may need this later */
home = (char*)opal_home_directory();

if(NULL == cwd) {
if (NULL == cwd) {
cwd = (char *) malloc(sizeof(char) * MAXPATHLEN);
if( NULL == (cwd = getcwd(cwd, MAXPATHLEN) )) {
opal_output(0, "Error: Unable to get the current working directory\n");
Expand All @@ -452,7 +414,8 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_2,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_var_files);
free (tmp);
if (OPAL_SUCCESS != ret) {
if (0 > ret) {
opal_output(0, "FAILED PARAM FILES: %d", ret);
return ret;
}

Expand All @@ -464,6 +427,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
ret = asprintf(&mca_base_var_override_file, "%s" OPAL_PATH_SEP "openmpi-mca-params-override.conf",
opal_install_dirs.sysconfdir);
if (0 > ret) {
opal_output(0, "FAILED OVERRIDE");
return OPAL_ERR_OUT_OF_RESOURCE;
}

Expand All @@ -476,6 +440,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
&mca_base_var_override_file);
free (tmp);
if (0 > ret) {
opal_output(0, "FAILED OVERRIDE PARAM FILES");
return ret;
}

Expand All @@ -490,6 +455,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_TYPE_BOOL, NULL, 0, 0, OPAL_INFO_LVL_2,
MCA_BASE_VAR_SCOPE_LOCAL, &mca_base_var_suppress_override_warning);
if (0 > ret) {
opal_output(0, "FAILED OVERRIDE WARNING");
return ret;
}

Expand All @@ -503,6 +469,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_var_file_prefix);
if (0 > ret) {
opal_output(0, "FAILED PARAM PREFIX");
return ret;
}

Expand All @@ -512,12 +479,14 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_envar_file_prefix);
if (0 > ret) {
opal_output(0, "FAILED ENV PREFIX");
return ret;
}

ret = asprintf(&mca_base_param_file_path, "%s" OPAL_PATH_SEP "amca-param-sets%c%s",
opal_install_dirs.opaldatadir, OPAL_ENV_SEP, cwd);
if (0 > ret) {
opal_output(0, "FAILED PARAM FILE PATH");
return OPAL_ERR_OUT_OF_RESOURCE;
}

Expand All @@ -528,6 +497,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_param_file_path);
free (tmp);
if (0 > ret) {
opal_output(0, "FAILED PARAM FILE PATH2");
return ret;
}

Expand All @@ -537,6 +507,7 @@ static int mca_base_var_cache_files(bool rel_path_search)
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &force_agg_path);
if (0 > ret) {
opal_output(0, "FAILED PARAM FILE FORCE");
return ret;
}

Expand Down
11 changes: 11 additions & 0 deletions opal/mca/base/mca_base_var.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,17 @@ OPAL_DECLSPEC int mca_base_var_dump(int vari, char ***out, mca_base_var_dump_typ
OPAL_DECLSPEC int mca_base_var_process_env_list(char *list, char ***argv);
OPAL_DECLSPEC int mca_base_var_process_env_list_from_file(char ***argv);

/*
* Initialize any file-based params
*/
OPAL_DECLSPEC int mca_base_var_cache_files(bool rel_path_search);


extern char *mca_base_env_list;
#define MCA_BASE_ENV_LIST_SEP_DEFAULT ";"
extern char *mca_base_env_list_sep;
extern char *mca_base_env_list_internal;

END_C_DECLS

#endif /* OPAL_MCA_BASE_VAR_H */
6 changes: 3 additions & 3 deletions opal/runtime/opal_info_support.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2006-2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2010-2015 Los Alamos National Security, LLC.
* All rights reserved.
* Copyright (c) 2010-2016 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2011-2012 University of Houston. All rights reserved.
* $COPYRIGHT$
*
Expand Down Expand Up @@ -174,7 +174,7 @@ int opal_info_init(int argc, char **argv,
}

/* Do the parsing */
ret = opal_cmd_line_parse(opal_info_cmd_line, false, argc, argv);
ret = opal_cmd_line_parse(opal_info_cmd_line, false, false, argc, argv);
if (OPAL_SUCCESS != ret) {
cmd_error = true;
if (OPAL_ERR_SILENT != ret) {
Expand Down
8 changes: 8 additions & 0 deletions opal/runtime/opal_init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "opal/util/proc.h"
#include "opal/memoryhooks/memory.h"
#include "opal/mca/base/base.h"
#include "opal/mca/base/mca_base_var.h"
#include "opal/runtime/opal.h"
#include "opal/util/net.h"
#include "opal/datatype/opal_datatype.h"
Expand Down Expand Up @@ -414,6 +415,13 @@ opal_init(int* pargc, char*** pargv)
return ret;
}

/* read any param files that were provided */
if (OPAL_SUCCESS != (ret = mca_base_var_cache_files(false))) {
error = "failed to cache files";
goto return_error;
}


/* open hwloc - since this is a static framework, no
* select is required
*/
Expand Down
40 changes: 38 additions & 2 deletions opal/runtime/opal_params.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "opal/mca/base/mca_base_var.h"
#include "opal/runtime/opal_params.h"
#include "opal/dss/dss.h"
#include "opal/util/opal_environ.h"
#include "opal/util/show_help.h"
#include "opal/util/timings.h"

Expand Down Expand Up @@ -292,7 +293,7 @@ int opal_register_params(void)
MCA_BASE_VAR_SCOPE_READONLY,
&opal_abort_delay);
if (0 > ret) {
return ret;
return ret;
}

opal_abort_print_stack = false;
Expand All @@ -313,9 +314,44 @@ int opal_register_params(void)
#endif
&opal_abort_print_stack);
if (0 > ret) {
return ret;
return ret;
}

/* register the envar-forwarding params */
(void)mca_base_var_register ("opal", "mca", "base", "env_list",
"Set SHELL env variables",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list);

mca_base_env_list_sep = MCA_BASE_ENV_LIST_SEP_DEFAULT;
(void)mca_base_var_register ("opal", "mca", "base", "env_list_delimiter",
"Set SHELL env variables delimiter. Default: semicolon ';'",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, 0, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list_sep);

/* Set OMPI_MCA_mca_base_env_list variable, it might not be set before
* if mca variable was taken from amca conf file. Need to set it
* here because mca_base_var_process_env_list is called from schizo_ompi.c
* only when this env variable was set.
*/
if (NULL != mca_base_env_list) {
char *name = NULL;
(void) mca_base_var_env_name ("mca_base_env_list", &name);
if (NULL != name) {
opal_setenv(name, mca_base_env_list, false, &environ);
free(name);
}
}

/* Register internal MCA variable mca_base_env_list_internal. It can be set only during
* parsing of amca conf file and contains SHELL env variables specified via -x there.
* Its format is the same as for mca_base_env_list.
*/
(void)mca_base_var_register ("opal", "mca", "base", "env_list_internal",
"Store SHELL env variables from amca conf file",
MCA_BASE_VAR_TYPE_STRING, NULL, 0, MCA_BASE_VAR_FLAG_INTERNAL, OPAL_INFO_LVL_3,
MCA_BASE_VAR_SCOPE_READONLY, &mca_base_env_list_internal);

/* The ddt engine has a few parameters */
ret = opal_datatype_register_params();
if (OPAL_SUCCESS != ret) {
Expand Down
4 changes: 2 additions & 2 deletions opal/tools/opal-checkpoint/opal-checkpoint.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2007-2013 Los Alamos National Security, LLC. All rights
* Copyright (c) 2007-2016 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2011-2012 Cisco Systems, Inc. All rights reserved.
* $COPYRIGHT$
Expand Down Expand Up @@ -283,7 +283,7 @@ static int parse_args(int argc, char *argv[]) {
opal_cmd_line_create(&cmd_line, cmd_line_opts);
mca_base_open();
mca_base_cmd_line_setup(&cmd_line);
ret = opal_cmd_line_parse(&cmd_line, true, argc, argv);
ret = opal_cmd_line_parse(&cmd_line, true, false, argc, argv);

if (OPAL_SUCCESS != ret) {
if (OPAL_ERR_SILENT != ret) {
Expand Down
4 changes: 2 additions & 2 deletions opal/tools/opal-restart/opal-restart.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2007-2013 Los Alamos National Security, LLC. All rights
* Copyright (c) 2007-2016 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2007 Evergrid, Inc. All rights reserved.
* Copyright (c) 2011 Oak Ridge National Labs. All rights reserved.
Expand Down Expand Up @@ -522,7 +522,7 @@ static int parse_args(int argc, char *argv[])

mca_base_open();
mca_base_cmd_line_setup(&cmd_line);
ret = opal_cmd_line_parse(&cmd_line, false, argc, argv);
ret = opal_cmd_line_parse(&cmd_line, false, false, argc, argv);
if (OPAL_SUCCESS != ret) {
if (OPAL_ERR_SILENT != ret) {
fprintf(stderr, "%s: command line error (%s)\n", argv[0],
Expand Down
11 changes: 6 additions & 5 deletions opal/util/cmd_line.c
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* -*- Mode: C; c-basic-offset:4 ; indent-tabs-mode:nil -*- */
/*
* Copyright (c) 2004-2005 The Trustees of Indiana University and Indiana
* University Research and Technology
Expand All @@ -9,8 +10,8 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2012 Los Alamos National Security, LLC.
* All rights reserved.
* Copyright (c) 2012-2016 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2012-2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
Expand Down Expand Up @@ -239,7 +240,7 @@ int opal_cmd_line_make_opt3(opal_cmd_line_t *cmd, char short_name,
* Parse a command line according to a pre-built OPAL command line
* handle.
*/
int opal_cmd_line_parse(opal_cmd_line_t *cmd, bool ignore_unknown,
int opal_cmd_line_parse(opal_cmd_line_t *cmd, bool ignore_unknown, bool ignore_unknown_option,
int argc, char **argv)
{
int i, j, orig, ret;
Expand Down Expand Up @@ -345,7 +346,7 @@ int opal_cmd_line_parse(opal_cmd_line_t *cmd, bool ignore_unknown,

if (NULL != option) {
opal_argv_delete(&cmd->lcl_argc,
&cmd->lcl_argv, i,
&cmd->lcl_argv, i,
1 + num_args_used);
opal_argv_insert(&cmd->lcl_argv, i, shortsv);
cmd->lcl_argc = opal_argv_count(cmd->lcl_argv);
Expand Down Expand Up @@ -476,7 +477,7 @@ int opal_cmd_line_parse(opal_cmd_line_t *cmd, bool ignore_unknown,
into the tail. If we're not ignoring unknowns, then print
an error and return. */
if (is_unknown_option || is_unknown_token) {
if (!ignore_unknown || is_unknown_option) {
if (!ignore_unknown || (is_unknown_option && !ignore_unknown_option)) {
fprintf(stderr, "%s: Error: unknown option \"%s\"\n",
cmd->lcl_argv[0], cmd->lcl_argv[i]);
printed_error = true;
Expand Down
Loading

3 comments on commit 42ecffb

@ggouaillardet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhc54 this commits causes some warnings at compilation

  BUILDING util/hostfile/orte_hosts.7
../../../src/ompi-master/orte/orted/orted_submit.c: In function 'orte_submit_init':
../../../src/ompi-master/orte/orted/orted_submit.c:278:9: warning: passing argument 2 of 'opal_cmd_line_add' from incompatible pointer type [enabled by default]
         if (OPAL_SUCCESS != (rc = opal_cmd_line_add(orte_cmd_line, opts))) {
         ^
In file included from ../../../src/ompi-master/opal/mca/base/base.h:39:0,
                 from ../../../src/ompi-master/opal/mca/event/event.h:31,
                 from ../../../src/ompi-master/orte/orted/orted_submit.c:60:
../../../src/ompi-master/opal/util/cmd_line.h:281:24: note: expected 'struct opal_cmd_line_init_t *' but argument is of type 'struct opal_cmd_line_t *'
      OPAL_DECLSPEC int opal_cmd_line_add(opal_cmd_line_t *cmd,

a struct opal_cmd_line_t * is used when a 'struct opal_cmd_line_init_t * pointer is expected.
these are clearly incompatible pointers (opal_cmd_line_t is an object, opal_cmd_line_init_t is not) so this is very likely a bug

could you please have a look at it ?

@ggouaillardet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hjelmn can you please also have a look at it ?
though @rhc54 commited the message, the log says you are the author

@rhc54
Copy link
Contributor

@rhc54 rhc54 commented on 42ecffb May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for it is sitting in #1685

Just waiting for a touch up change from @hjelmn

Please sign in to comment.