From 2aa5f92fca7b1e7efd8a4e58b7c664e367752de8 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Tue, 13 Jul 2004 20:24:26 +0000 Subject: [PATCH] - Code review of the registration / lookup code with George - Various fixes / cleanups as a result of the code review George makes a good point that we could consolidate a lot of the malloc's / strdup's in this code for platforms where we do not have a lot of memory (i.e., add up all the memory required and make one big malloc vs. lots of little mallocs). This is something for post-first-release. - Convert another data structure to the OBJ_* interface. - Split the internal prototypes and struct definitions into their own header file that won't be included anywhere else in Open MPI (except ompi_info). This commit was SVN r1692. --- src/mca/base/Makefile.am | 1 + src/mca/base/mca_base_param.c | 218 +++++++++++++++---------- src/mca/base/mca_base_param.h | 134 +++++---------- src/mca/base/mca_base_param_internal.h | 83 ++++++++++ 4 files changed, 258 insertions(+), 178 deletions(-) create mode 100644 src/mca/base/mca_base_param_internal.h diff --git a/src/mca/base/Makefile.am b/src/mca/base/Makefile.am index 459be7a985..99ee244a0a 100644 --- a/src/mca/base/Makefile.am +++ b/src/mca/base/Makefile.am @@ -17,6 +17,7 @@ headers = \ base.h \ mca_base_module_exchange.h \ mca_base_param.h \ + mca_base_param_internal.h \ mca_base_msgbuf.h \ mca_base_msgbuf_internal.h diff --git a/src/mca/base/mca_base_param.c b/src/mca/base/mca_base_param.c index 296b0e486b..f61c731bc5 100644 --- a/src/mca/base/mca_base_param.c +++ b/src/mca/base/mca_base_param.c @@ -14,13 +14,14 @@ #include "attribute/attribute.h" #include "mca/mca.h" #include "mca/base/mca_base_param.h" +#include "mca/base/mca_base_param_internal.h" /* * Public variables * * This variable is public, but not advertised in mca_base_param.h. - * It's only public so that ompi_info can see it. The relevant module + * It's only public so that ompi_info can see it. The relevant component * in ompi_info will provide an extern to see this variable. */ ompi_value_array_t mca_base_params; @@ -36,21 +37,29 @@ static bool initialized = false; /* * local functions */ -static int param_register(const char *type_name, const char *module_name, +static int param_register(const char *type_name, const char *component_name, const char *param_name, const char *mca_param_name, mca_base_param_type_t type, mca_base_param_storage_t *default_value); static bool param_lookup(int index, mca_base_param_storage_t *storage, ompi_hash_table_t *attrs); -static void param_free(mca_base_param_t *p); +static void param_constructor(mca_base_param_t *p); +static void param_destructor(mca_base_param_t *p); + + +/* + * Make the class instance for mca_base_param_t + */ +OBJ_CLASS_INSTANCE(mca_base_param_t, ompi_object_t, + param_constructor, param_destructor); /* * Register an integer MCA parameter */ int mca_base_param_register_int(const char *type_name, - const char *module_name, + const char *component_name, const char *param_name, const char *mca_param_name, int default_value) @@ -58,7 +67,7 @@ int mca_base_param_register_int(const char *type_name, mca_base_param_storage_t storage; storage.intval = default_value; - return param_register(type_name, module_name, param_name, mca_param_name, + return param_register(type_name, component_name, param_name, mca_param_name, MCA_BASE_PARAM_TYPE_INT, &storage); } @@ -67,7 +76,7 @@ int mca_base_param_register_int(const char *type_name, * Register a string MCA parameter. */ int mca_base_param_register_string(const char *type_name, - const char *module_name, + const char *component_name, const char *param_name, const char *mca_param_name, const char *default_value) @@ -78,7 +87,7 @@ int mca_base_param_register_string(const char *type_name, } else { storage.stringval = NULL; } - return param_register(type_name, module_name, param_name, mca_param_name, + return param_register(type_name, component_name, param_name, mca_param_name, MCA_BASE_PARAM_TYPE_STRING, &storage); } @@ -177,7 +186,7 @@ int mca_base_param_kv_lookup_string(int index, ompi_hash_table_t *attrs, /* * Find the index for an MCA parameter based on its names. */ -int mca_base_param_find(const char *type_name, const char *module_name, +int mca_base_param_find(const char *type_name, const char *component_name, const char *param_name) { size_t i, size; @@ -188,21 +197,23 @@ int mca_base_param_find(const char *type_name, const char *module_name, if (!initialized) { return OMPI_ERROR; } - if (NULL == type_name || NULL == param_name) { + if (NULL == type_name) { return OMPI_ERROR; } /* Loop through looking for a parameter of a given - type/module/param */ + type/component/param */ size = ompi_value_array_get_size(&mca_base_params); array = OMPI_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t); for (i = 0; i < size; ++i) { if (0 == strcmp(type_name, array[i].mbp_type_name) && - ((NULL == module_name && NULL == array[i].mbp_module_name) || - (NULL != module_name && NULL != array[i].mbp_module_name && - 0 == strcmp(module_name, array[i].mbp_module_name))) && - 0 == strcmp(param_name, array[i].mbp_param_name)) { + ((NULL == component_name && NULL == array[i].mbp_component_name) || + (NULL != component_name && NULL != array[i].mbp_component_name && + 0 == strcmp(component_name, array[i].mbp_component_name))) && + ((NULL == param_name && NULL == array[i].mbp_param_name) || + (NULL != param_name && NULL != array[i].mbp_param_name && + 0 == strcmp(param_name, array[i].mbp_param_name)))) { return i; } } @@ -222,9 +233,12 @@ int mca_base_param_finalize(void) mca_base_param_t *array; if (initialized) { + + /* This is slow, but effective :-) */ + array = OMPI_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t); while (0 < ompi_value_array_get_size(&mca_base_params)) { - param_free(&array[0]); + OBJ_DESTRUCT(&array[0]); ompi_value_array_remove_item(&mca_base_params, 0); } OBJ_DESTRUCT(&mca_base_params); @@ -237,11 +251,12 @@ int mca_base_param_finalize(void) /*************************************************************************/ -static int param_register(const char *type_name, const char *module_name, +static int param_register(const char *type_name, const char *component_name, const char *param_name, const char *mca_param_name, mca_base_param_type_t type, mca_base_param_storage_t *default_value) { + int ret; size_t i, len; mca_base_param_t param, *array; @@ -253,31 +268,38 @@ static int param_register(const char *type_name, const char *module_name, initialized = true; } + /* Error check */ + + if (NULL == type_name) { + return OMPI_ERR_BAD_PARAM; + } + /* Create a parameter entry. If a keyval is to be used, it will be registered elsewhere. We simply assign -1 here. */ + OBJ_CONSTRUCT(¶m, mca_base_param_t); param.mbp_type = type; - param.mbp_keyval = -1; + param.mbp_keyval = MPI_KEYVAL_INVALID; param.mbp_type_name = strdup(type_name); if (NULL == param.mbp_type_name) { - return OMPI_ERROR; + OBJ_DESTRUCT(¶m); + return OMPI_ERR_OUT_OF_RESOURCE; } - if (NULL != module_name) { - param.mbp_module_name = strdup(module_name); - if (NULL == param.mbp_module_name) { - free(param.mbp_type_name); - return OMPI_ERROR; + if (NULL != component_name) { + param.mbp_component_name = strdup(component_name); + if (NULL == param.mbp_component_name) { + OBJ_DESTRUCT(¶m); + return OMPI_ERR_OUT_OF_RESOURCE; } } else { - param.mbp_module_name = NULL; + param.mbp_param_name = NULL; } - if (param_name != NULL) { + if (NULL != param_name) { param.mbp_param_name = strdup(param_name); if (NULL == param.mbp_param_name) { - free(param.mbp_type_name); - free(param.mbp_module_name); - return OMPI_ERROR; + OBJ_DESTRUCT(¶m); + return OMPI_ERR_OUT_OF_RESOURCE; } } else { param.mbp_param_name = NULL; @@ -285,17 +307,20 @@ static int param_register(const char *type_name, const char *module_name, /* The full parameter name may have been specified by the caller. If it was, use that (only for backwards compatability). - Otherwise, derive it from the type, module, and parameter + Otherwise, derive it from the type, component, and parameter name. */ param.mbp_env_var_name = NULL; - if (MCA_BASE_PARAM_INFO != mca_param_name && NULL != mca_param_name) { + if (NULL != mca_param_name) { param.mbp_full_name = strdup(mca_param_name); + if (NULL == param.mbp_full_name) { + OBJ_DESTRUCT(¶m); + return OMPI_ERROR; + } } else { len = 16 + strlen(type_name); - - if (NULL != module_name) { - len += strlen(module_name); + if (NULL != component_name) { + len += strlen(component_name); } if (NULL != param_name) { len += strlen(param_name); @@ -303,22 +328,16 @@ static int param_register(const char *type_name, const char *module_name, param.mbp_full_name = malloc(len); if (NULL == param.mbp_full_name) { - if (NULL != param.mbp_type_name) { - free(param.mbp_type_name); - } - if (NULL != param.mbp_module_name) { - free(param.mbp_module_name); - } - if (NULL != param.mbp_param_name) { - free(param.mbp_param_name); - } + OBJ_DESTRUCT(¶m); return OMPI_ERROR; } - strncpy(param.mbp_full_name, type_name, len); - if (NULL != module_name) { + /* Copy the name over in parts */ + + strncpy(param.mbp_full_name, type_name, len); + if (NULL != component_name) { strcat(param.mbp_full_name, "_"); - strcat(param.mbp_full_name, module_name); + strcat(param.mbp_full_name, component_name); } if (NULL != param_name) { strcat(param.mbp_full_name, "_"); @@ -326,25 +345,19 @@ static int param_register(const char *type_name, const char *module_name, } } - /* If mca_param_name isn't MCA_BASE_PARAM_INFO, then it's a - lookup-able value. So alloc the environment variable name as - well. */ + /* Create the environment name */ - if (MCA_BASE_PARAM_INFO != mca_param_name) { - len = strlen(param.mbp_full_name) + strlen(mca_prefix) + 16; - param.mbp_env_var_name = malloc(len); - if (NULL == param.mbp_env_var_name) { - free(param.mbp_full_name); - free(param.mbp_type_name); - free(param.mbp_module_name); - free(param.mbp_param_name); - return OMPI_ERROR; - } - snprintf(param.mbp_env_var_name, len, "%s%s", mca_prefix, - param.mbp_full_name); + len = strlen(param.mbp_full_name) + strlen(mca_prefix) + 16; + param.mbp_env_var_name = malloc(len); + if (NULL == param.mbp_env_var_name) { + OBJ_DESTRUCT(¶m); + return OMPI_ERROR; } + snprintf(param.mbp_env_var_name, len, "%s%s", mca_prefix, + param.mbp_full_name); - /* Figure out the default value */ + /* Figure out the default value; zero it out if a default is not + provided */ if (NULL != default_value) { if (MCA_BASE_PARAM_TYPE_STRING == param.mbp_type && @@ -357,49 +370,59 @@ static int param_register(const char *type_name, const char *module_name, memset(¶m.mbp_default_value, 0, sizeof(param.mbp_default_value)); } - /* See if this entry is already in the Array */ + /* See if this entry is already in the array */ len = ompi_value_array_get_size(&mca_base_params); array = OMPI_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t); for (i = 0; i < len; ++i) { if (0 == strcmp(param.mbp_full_name, array[i].mbp_full_name)) { - /* Copy in the new default value to the old entry */ + /* We found an entry with the same param name. Free the old + value (if it was a string */ if (MCA_BASE_PARAM_TYPE_STRING == array[i].mbp_type && NULL != array[i].mbp_default_value.stringval) { free(array[i].mbp_default_value.stringval); } - if (MCA_BASE_PARAM_TYPE_STRING == param.mbp_type && - NULL != param.mbp_default_value.stringval) { - array[i].mbp_default_value.stringval = - strdup(param.mbp_default_value.stringval); + + /* Now put in the new value */ + + if (MCA_BASE_PARAM_TYPE_STRING == param.mbp_type) { + if (NULL != param.mbp_default_value.stringval) { + array[i].mbp_default_value.stringval = + strdup(param.mbp_default_value.stringval); + } else { + array[i].mbp_default_value.stringval = NULL; + } + } else { + array[i].mbp_default_value.intval = + param.mbp_default_value.intval; } - param_free(¶m); + /* Just in case we changed type */ + + array[i].mbp_type = param.mbp_type; + + /* Now delete the newly-created entry (since we just saved the + value in the old entry) */ + + OBJ_DESTRUCT(¶m); return i; } } /* Add it to the array */ - if (OMPI_SUCCESS != ompi_value_array_append_item(&mca_base_params, ¶m)) { - return OMPI_ERROR; + if (OMPI_SUCCESS != + (ret = ompi_value_array_append_item(&mca_base_params, ¶m))) { + return ret; } return ompi_value_array_get_size(&mca_base_params) - 1; } /* - * DO NOT MODIFY THIS FUNCTION WITHOUT ALSO MODIFYING mca_base_param.c! - * - * This function appears in libompi. Because of unix linker semantics, - * it's simply easier to essentially duplicate this function in libmpi - * because in libmpi, we need to lookup on a keyval before looking in - * the environment. The logic is simpler if we just duplicate/alter - * the code in mca_base_param.c rather than try to make this a) public, - * and b) more general (to accomodate looking up keyvals while not - * linking to MPI_Comm_get_attr() in libmpi). + * Lookup a parameter */ static bool param_lookup(int index, mca_base_param_storage_t *storage, ompi_hash_table_t *attrs) @@ -439,13 +462,13 @@ static bool param_lookup(int index, mca_base_param_storage_t *storage, &storage->stringval, &flag); if (OMPI_SUCCESS == err && 1 == flag) { - /* Because of alignment weirdness between (void*) and int, it's - simpler to just call ompi_attr_get with the right storage - vehicle vs. trying to cast (extra) to (storage) */ + /* Because of alignment weirdness between (void*) and int, we + must grab the lower sizeof(int) bytes from the (char*) in + stringval, in case sizeof(int) != sizeof(char*). */ if (MCA_BASE_PARAM_TYPE_INT == array[index].mbp_type) { - err = ompi_attr_get(attrs, array[index].mbp_keyval, - &storage->intval, &flag); + storage->intval = *((int *) (storage->stringval + + sizeof(void *) - sizeof(int))); } /* Nothing to do for string -- we already have the value loaded @@ -494,13 +517,34 @@ static bool param_lookup(int index, mca_base_param_storage_t *storage, } -static void param_free(mca_base_param_t *p) +/* + * Create an empty param container + */ +static void param_constructor(mca_base_param_t *p) +{ + p->mbp_type = MCA_BASE_PARAM_TYPE_MAX; + + p->mbp_type_name = NULL; + p->mbp_component_name = NULL; + p->mbp_param_name = NULL; + p->mbp_full_name = NULL; + + p->mbp_keyval = -1; + p->mbp_env_var_name = NULL; + p->mbp_default_value.stringval = NULL; +} + + +/* + * Free all the contents of a param container + */ +static void param_destructor(mca_base_param_t *p) { if (NULL != p->mbp_type_name) { free(p->mbp_type_name); } - if (NULL != p->mbp_module_name) { - free(p->mbp_module_name); + if (NULL != p->mbp_component_name) { + free(p->mbp_component_name); } if (NULL != p->mbp_param_name) { free(p->mbp_param_name); diff --git a/src/mca/base/mca_base_param.h b/src/mca/base/mca_base_param.h index ef88fe9e2a..9c03c6eff1 100644 --- a/src/mca/base/mca_base_param.h +++ b/src/mca/base/mca_base_param.h @@ -19,6 +19,11 @@ * -# Lookup a "normal" parameter value on a specific index, or * -# Lookup an attribute parameter on a specific index and * communicator / datatype / window. + * + * Note that there is a second header file (mca_base_param_internal.h) + * that contains several internal type delcarations for the parameter + * system. The internal file is only used within the parameter system + * itself; it should not be required by any other Open MPI entities. */ #ifndef OMPI_MCA_BASE_PARAM_H @@ -28,76 +33,6 @@ #include "class/ompi_hash_table.h" -/** - * \internal - * - * Types for MCA parameters. - */ -typedef union { - int intval; - /**< Integer value */ - char *stringval; - /**< String value */ -} mca_base_param_storage_t; - -/** \internal - * - * Special name used to indicate that this is an "info" value. - */ -#define MCA_BASE_PARAM_INFO ((void*) -1) - - -/** - * \internal - * - * The following types are really in this public .h file so that - * ompi_info can see them. No one else should use them! - */ -typedef enum { - MCA_BASE_PARAM_TYPE_INT, - /**< The parameter is of type integer. */ - MCA_BASE_PARAM_TYPE_STRING, - /**< The parameter is of type string. */ - - MCA_BASE_PARAM_TYPE_MAX - /**< Maximum parameter type. */ -} mca_base_param_type_t; - -/** - * \internal - * - * Entry for holding the information about an MCA parameter and its - * default value. - */ -struct mca_base_param_t { - mca_base_param_type_t mbp_type; - /**< Enum indicating the type of the parameter (integer or string) */ - char *mbp_type_name; - /**< String of the type name, or NULL */ - char *mbp_module_name; - /**< String of the component name */ - char *mbp_param_name; - /**< String of the parameter name */ - char *mbp_full_name; - /**< Full parameter name, in case it is not - __ */ - - int mbp_keyval; - /**< Keyval value for MPI attribute parameters */ - char *mbp_env_var_name; - /**< Environment variable name */ - - mca_base_param_storage_t mbp_default_value; - /**< Default value of the parameter */ -}; -/** - * \internal - * - * Convenience typedef. - */ -typedef struct mca_base_param_t mca_base_param_t; - - /* * Global functions for MCA */ @@ -109,12 +44,11 @@ extern "C" { * Register an integer MCA parameter. * * @param type_name[in] The MCA type (string). - * @param module_name[in] The name of the module (string). + * @param component_name[in] The name of the component (string). * @param param_name[in] The name of the parameter being registered * (string). - * @param mca_param_name[in] If NULL, the user-visible name of the - * parameter is {type_name}_{module_name}_{param_name}. If this - * parameter is non-NULL, it is used instead of the default name. + * @param mca_param_name[in] Optional parameter to override the + * user-visible name of this parameter (string). * @param default_value[in] The value that is used for this * parameter if the user does not supply one. * @@ -123,13 +57,33 @@ extern "C" { * mca_base_param_lookup_int() to retrieve the value of the parameter. * * This function registers an integer MCA parameter and associates it - * with a specific module. + * with a specific component. * - * In most cases, mca_param_name should be NULL. Only in rare cases - * is it necessary (or advisable) to override the default name. + * The default resulting MCA parameter name is + * {type_name}[_{component_name}][_{param_name}]. + * + * {component_name} is only included if it is non-NULL. All + * components an should include their name; component frameworks + * should pass "base". It is only permissible for the MCA base + * itself to pass NULL for the component_name. + * + * Likewise, {param_name} is also only included if it is non-NULL. + * Components and frameworks can pass NULL for this parameter if + * they wish. + * + * In most cases, mca_param_name should be NULL, in which case the + * user-visible name of this parameter will be the default form (as + * described above). Only in rare cases is it necessary (or + * advisable) to override the default name -- its use is strongly + * discouraged. + * + * It is permissable to register a (type_name, component_name, + * param_name) triple more than once; the same index value will be + * returned, but the default value will be changed to reflect the + * last registration. */ int mca_base_param_register_int(const char *type_name, - const char *module_name, + const char *component_name, const char *param_name, const char *mca_param_name, int default_value); @@ -138,12 +92,11 @@ extern "C" { * Register a string MCA parameter. * * @param type_name[in] The MCA type (string). - * @param module_name[in] The name of the module (string). + * @param component_name[in] The name of the component (string). * @param param_name[in] The name of the parameter being registered * (string). - * @param mca_param_name[in] If NULL, the user-visible name of the - * parameter is {type_name}_{module_name}_{param_name}. If this - * parameter is non-NULL, it is used instead of the default name. + * @param mca_param_name[in] Optional parameter to override the + * user-visible name of this parameter (string). * @param default_value[in] The value that is used for this * parameter if the user does not supply one. * @@ -152,14 +105,13 @@ extern "C" { * mca_base_param_lookup_string() to retrieve the value of the * parameter. * - * This function registers an string MCA parameter and associates it - * with a specific module. - * - * In most cases, mca_param_name should be NULL. Only in rare cases - * is it necessary (or advisable) to override the default name. + * This function is identical to mca_base_param_register_int() + * except that you are registering a string parameter with an + * associated string default value (which is allowed to be NULL). + * See mca_base_param_register_int() for all other details. */ int mca_base_param_register_string(const char *type_name, - const char *module_name, + const char *component_name, const char *param_name, const char *mca_param_name, const char *default_value); @@ -275,7 +227,7 @@ extern "C" { * Find the index for an MCA parameter based on its names. * * @param type_name Name of the type containing the parameter. - * @param module_name Name of the module containing the parameter. + * @param component_name Name of the component containing the parameter. * @param param_name Name of the parameter. * * @retval OMPI_ERROR If the parameter was not found. @@ -283,14 +235,14 @@ extern "C" { * * It is not always convenient to widely propagate a parameter's index * value, or it may be necessary to look up the parameter from a - * different module -- where it is not possible to have the return + * different component -- where it is not possible to have the return * value from mca_base_param_register_int() or * mca_base_param_register_string(). This function can be used to * look up the index of any registered parameter. The returned index * can be used with mca_base_param_lookup_int() and * mca_base_param_lookup_string(). */ - int mca_base_param_find(const char *type, const char *module, + int mca_base_param_find(const char *type, const char *component, const char *param); /** diff --git a/src/mca/base/mca_base_param_internal.h b/src/mca/base/mca_base_param_internal.h new file mode 100644 index 0000000000..fe869a0d7a --- /dev/null +++ b/src/mca/base/mca_base_param_internal.h @@ -0,0 +1,83 @@ +/* + * $HEADER$ + */ + +/** @file This is the private declarations for the MCA parameter + * system. This file is internal to the MCA parameter system and + * should not need to be used by any other elements in Open MPI except + * the special case of the ompi_info command. + */ + +#ifndef OMPI_MCA_BASE_PARAM_INTERNAL_H +#define OMPI_MCA_BASE_PARAM_INTERNAL_H + +#include "mpi.h" + +#include "class/ompi_object.h" +#include "class/ompi_hash_table.h" + +/** + * Types for MCA parameters. + */ +typedef union { + int intval; + /**< Integer value */ + char *stringval; + /**< String value */ +} mca_base_param_storage_t; + + +/** + * The following types are really in this public .h file so that + * ompi_info can see them. No one else should use them! + */ +typedef enum { + MCA_BASE_PARAM_TYPE_INT, + /**< The parameter is of type integer. */ + MCA_BASE_PARAM_TYPE_STRING, + /**< The parameter is of type string. */ + + MCA_BASE_PARAM_TYPE_MAX + /**< Maximum parameter type. */ +} mca_base_param_type_t; + +/** + * Entry for holding the information about an MCA parameter and its + * default value. + */ +struct mca_base_param_t { + ompi_object_t mbp_super; + /**< Allow this to be an OMPI OBJ */ + + mca_base_param_type_t mbp_type; + /**< Enum indicating the type of the parameter (integer or string) */ + char *mbp_type_name; + /**< String of the type name, or NULL */ + char *mbp_component_name; + /**< String of the component name */ + char *mbp_param_name; + /**< String of the parameter name */ + char *mbp_full_name; + /**< Full parameter name, in case it is not + __ */ + + int mbp_keyval; + /**< Keyval value for MPI attribute parameters */ + char *mbp_env_var_name; + /**< Environment variable name */ + + mca_base_param_storage_t mbp_default_value; + /**< Default value of the parameter */ +}; +/** + * Convenience typedef. + */ +typedef struct mca_base_param_t mca_base_param_t; + +/** + * Object delcataion for mca_base_param_t + */ +OBJ_CLASS_DECLARATION(mca_base_param_t); + + +#endif /* OMPI_MCA_BASE_PARAM_INTERNAL_H */