[ofa-general] Re: [opensm patch][1/2] fix qos config parsing bugs
Sasha Khapyorsky
sashak at voltaire.com
Wed Nov 12 16:24:03 PST 2008
Hi Al,
On 15:57 Tue 11 Nov , Al Chu wrote:
>
> Sorry, I may have not explained it well. Lets say I do this in the
> config file.
>
> qos_vlarb_high FOOBAR
> # qos_ca_vlarb_high BLAH
> qos_swe_vlarb_high XYZZY
>
> I currently expect qos_ca_vlarb_high to use the value of FOOBAR because
> I commented out the field. But it uses OSM_DEFAULT_QOS_HIGH_LIMIT
> instead. The reason is because qos_build_config() checks for NULL to
> use default vs. non-default values.
>
> p = opt->vlarb_high ? opt->vlarb_high : dflt->vlarb_high;
>
> Under the above situation where I've commented out veral fields, opt-
> >vlarb_high is always non-NULL b/c it was set to
> OSM_DEFAULT_QOS_HIGH_LIMIT. Thus OSM_DEFAULT_QOS_HIGH_LIMIT is used
> instead of FOOBAR.
>
> > > 2)
> > >
> > > In qos_build_config() we load the high_limit like this:
> > >
> > > cfg->vl_high_limit = (uint8_t) opt->high_limit;
> > >
> > > So there is no way to tell the qos_ca, qos_swe, qos_rtr, etc. high_limit
> > > options to "go back to" the default high_limit. It just assumes that
> > > whatever is input (or was set by default) is what you should use.
> >
> > Right. What is a limitation here? That an user cannot set this to
> > "no value"? But she/he can just skip it.
>
> Similar to the above issue, lets say I want to do:
>
> qos_high_limit 8
> # qos_ca_high_limit 15
> # qos_swe_high_limit 15
>
> I want qos_ca_high_limit and qos_swe_high_limit to use whatever I set in
> qos_high_limit. But the code doesn't allow for this.
>
> >
> > > 3)
> > >
> > > Some fields like qos_vlarb_high are assumed to be correctly set and can
> > > segfault opensm.
> >
> > qos_build_config() assumes that valid parameters are used. And we are
> > using this this way (I hope :)) (finally it is not library API).
>
> I think the issue is the osm_subnet.c code did not properly check all
> inputs, and subsequently some inputs used in qos_build_config() were
> bad. I think
>
> qos_vlarb_high (null)
>
> was something I tried that opensm seg-faulted on.
Ok. I see now.
Probably it will be simpler just to generate a valid qos parameter sets
right after parser (in verification time)? Like in your modified (and
rebased against recent patches) patch below?
Sasha
>From a973a8a1ea6c805cf07965d86731ae04510266ce Mon Sep 17 00:00:00 2001
From: Al Chu <chu11 at llnl.gov>
Date: Mon, 10 Nov 2008 13:41:04 -0800
Subject: [PATCH] fix qos config parsing bugs
Signed-off-by: Albert Chu <chu11 at llnl.gov>
Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
---
opensm/include/opensm/osm_subnet.h | 12 +-
opensm/opensm/osm_qos.c | 6 +-
opensm/opensm/osm_subnet.c | 298 ++++++++++++++++++++---------------
3 files changed, 181 insertions(+), 135 deletions(-)
diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
index a16cbce..2bcd232 100644
--- a/opensm/include/opensm/osm_subnet.h
+++ b/opensm/include/opensm/osm_subnet.h
@@ -100,7 +100,7 @@ struct osm_qos_policy;
*/
typedef struct osm_qos_options {
unsigned max_vls;
- unsigned high_limit;
+ int high_limit;
char *vlarb_high;
char *vlarb_low;
char *sl2vl;
@@ -109,20 +109,20 @@ typedef struct osm_qos_options {
* FIELDS
*
* max_vls
-* The number of maximum VLs on the Subnet
+* The number of maximum VLs on the Subnet (0 == use default)
*
* high_limit
* The limit of High Priority component of VL Arbitration
-* table (IBA 7.6.9)
+* table (IBA 7.6.9) (-1 == use default)
*
* vlarb_high
-* High priority VL Arbitration table template.
+* High priority VL Arbitration table template. (NULL == use default)
*
* vlarb_low
-* Low priority VL Arbitration table template.
+* Low priority VL Arbitration table template. (NULL == use default)
*
* sl2vl
-* SL2VL Mapping table (IBA 7.6.6) template.
+* SL2VL Mapping table (IBA 7.6.6) template. (NULL == use default)
*
*********/
diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
index 1679ae0..b451c25 100644
--- a/opensm/opensm/osm_qos.c
+++ b/opensm/opensm/osm_qos.c
@@ -382,7 +382,11 @@ static void qos_build_config(struct qos_config *cfg,
memset(cfg, 0, sizeof(*cfg));
cfg->max_vls = opt->max_vls > 0 ? opt->max_vls : dflt->max_vls;
- cfg->vl_high_limit = (uint8_t) opt->high_limit;
+
+ if (opt->high_limit >= 0)
+ cfg->vl_high_limit = (uint8_t) opt->high_limit;
+ else
+ cfg->vl_high_limit = (uint8_t) dflt->high_limit;
p = opt->vlarb_high ? opt->vlarb_high : dflt->vlarb_high;
for (i = 0; i < 2 * IB_NUM_VL_ARB_ELEMENTS_IN_BLOCK; i++) {
diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
index 8569043..1c9777e 100644
--- a/opensm/opensm/osm_subnet.c
+++ b/opensm/opensm/osm_subnet.c
@@ -370,6 +370,15 @@ static void subn_set_default_qos_options(IN osm_qos_options_t * opt)
opt->sl2vl = OSM_DEFAULT_QOS_SL2VL;
}
+static void subn_init_qos_options(IN osm_qos_options_t * opt)
+{
+ opt->max_vls = 0;
+ opt->high_limit = -1;
+ opt->vlarb_high = NULL;
+ opt->vlarb_low = NULL;
+ opt->sl2vl = NULL;
+}
+
/**********************************************************************
**********************************************************************/
void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
@@ -457,11 +466,11 @@ void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
p_opt->no_clients_rereg = FALSE;
p_opt->prefix_routes_file = OSM_DEFAULT_PREFIX_ROUTES_FILE;
p_opt->consolidate_ipv6_snm_req = FALSE;
- subn_set_default_qos_options(&p_opt->qos_options);
- subn_set_default_qos_options(&p_opt->qos_ca_options);
- subn_set_default_qos_options(&p_opt->qos_sw0_options);
- subn_set_default_qos_options(&p_opt->qos_swe_options);
- subn_set_default_qos_options(&p_opt->qos_rtr_options);
+ subn_init_qos_options(&p_opt->qos_options);
+ subn_init_qos_options(&p_opt->qos_ca_options);
+ subn_init_qos_options(&p_opt->qos_sw0_options);
+ subn_init_qos_options(&p_opt->qos_swe_options);
+ subn_init_qos_options(&p_opt->qos_rtr_options);
}
/**********************************************************************
@@ -526,6 +535,21 @@ opts_unpack_uint32(IN char *p_req_key,
/**********************************************************************
**********************************************************************/
static void
+opts_unpack_int32(IN char *p_req_key,
+ IN char *p_key, IN char *p_val_str, IN int32_t * p_val)
+{
+ if (!strcmp(p_req_key, p_key)) {
+ int32_t val = strtol(p_val_str, NULL, 0);
+ if (val != *p_val) {
+ log_config_value(p_key, "%d", val);
+ *p_val = val;
+ }
+ }
+}
+
+/**********************************************************************
+ **********************************************************************/
+static void
opts_unpack_uint16(IN char *p_req_key,
IN char *p_key, IN char *p_val_str, IN uint16_t * p_val)
{
@@ -651,7 +675,7 @@ subn_parse_qos_options(IN const char *prefix,
snprintf(name, sizeof(name), "%s_max_vls", prefix);
opts_unpack_uint32(name, p_key, p_val_str, &opt->max_vls);
snprintf(name, sizeof(name), "%s_high_limit", prefix);
- opts_unpack_uint32(name, p_key, p_val_str, &opt->high_limit);
+ opts_unpack_int32(name, p_key, p_val_str, &opt->high_limit);
snprintf(name, sizeof(name), "%s_vlarb_high", prefix);
opts_unpack_charp(name, p_key, p_val_str, &opt->vlarb_high);
snprintf(name, sizeof(name), "%s_vlarb_low", prefix);
@@ -786,138 +810,142 @@ osm_parse_prefix_routes_file(IN osm_subn_t * const p_subn)
/**********************************************************************
**********************************************************************/
-
-static void subn_verify_max_vls(unsigned *max_vls, const char *prefix)
+static void subn_verify_max_vls(unsigned *max_vls, const char *prefix, unsigned dflt)
{
- if (*max_vls > 15) {
- log_report(" Invalid Cached Option:%s_max_vls=%u:"
- "Using Default:%u\n",
- prefix, *max_vls, OSM_DEFAULT_QOS_MAX_VLS);
- *max_vls = OSM_DEFAULT_QOS_MAX_VLS;
+ if (!(*max_vls) || *max_vls > 15) {
+ log_report(" Invalid Cached Option: %s_max_vls=%u: "
+ "Using Default = %u\n", prefix, *max_vls, dflt);
+ *max_vls = dflt;
}
}
-static void subn_verify_high_limit(unsigned *high_limit, const char *prefix)
+static void subn_verify_high_limit(int *high_limit, const char *prefix, int dflt)
{
- if (*high_limit > 255) {
- log_report(" Invalid Cached Option:%s_high_limit=%u:"
- "Using Default:%u\n",
- prefix, *high_limit, OSM_DEFAULT_QOS_HIGH_LIMIT);
- *high_limit = OSM_DEFAULT_QOS_HIGH_LIMIT;
+ if (*high_limit < 0 || *high_limit > 255) {
+ log_report(" Invalid Cached Option: %s_high_limit=%d: "
+ "Using Default: %d\n", prefix, *high_limit, dflt);
+ *high_limit = dflt;
}
}
-static void subn_verify_vlarb(char *vlarb, const char *prefix,
- const char *suffix)
+static void subn_verify_vlarb(char **vlarb, const char *prefix,
+ const char *suffix, char *dflt)
{
- if (vlarb) {
- char *str, *tok, *end, *ptr;
- int count = 0;
-
- str = strdup(vlarb);
-
- tok = strtok_r(str, ",\n", &ptr);
- while (tok) {
- char *vl_str, *weight_str;
-
- vl_str = tok;
- weight_str = strchr(tok, ':');
-
- if (weight_str) {
- long vl, weight;
-
- *weight_str = '\0';
- weight_str++;
-
- vl = strtol(vl_str, &end, 0);
-
- if (*end)
- log_report(" Warning: Cached Option "
- "%s_vlarb_%s:vl=%s "
- "improperly formatted\n",
- prefix, suffix, vl_str);
- else if (vl < 0 || vl > 14)
- log_report(" Warning: Cached Option "
- "%s_vlarb_%s:vl=%ld out "
- "of range\n",
- prefix, suffix, vl);
-
- weight = strtol(weight_str, &end, 0);
-
- if (*end)
- log_report(" Warning: Cached Option "
- "%s_vlarb_%s:weight=%s "
- "improperly formatted\n",
- prefix, suffix, weight_str);
- else if (weight < 0 || weight > 255)
- log_report(" Warning: Cached Option "
- "%s_vlarb_%s:weight=%ld "
- "out of range\n",
- prefix, suffix, weight);
- } else
- log_report(" Warning: Cached Option "
- "%s_vlarb_%s:vl:weight=%s "
- "improperly formatted\n",
- prefix, suffix, tok);
+ char *str, *tok, *end, *ptr;
+ int count = 0;
+
+ if (*vlarb == NULL) {
+ log_report(" Invalid Cached Option: %s_vlarb_%s: "
+ "Using Default\n", prefix, suffix);
+ *vlarb = dflt;
+ return;
+ }
- count++;
- tok = strtok_r(NULL, ",\n", &ptr);
- }
+ str = strdup(*vlarb);
+
+ tok = strtok_r(str, ",\n", &ptr);
+ while (tok) {
+ char *vl_str, *weight_str;
- if (count > 64)
- log_report(" Warning: Cached Option %s_vlarb_%s: "
- "> 64 listed: excess vl:weight pairs "
- "will be dropped\n", prefix, suffix);
+ vl_str = tok;
+ weight_str = strchr(tok, ':');
- free(str);
+ if (weight_str) {
+ long vl, weight;
+
+ *weight_str = '\0';
+ weight_str++;
+
+ vl = strtol(vl_str, &end, 0);
+
+ if (*end)
+ log_report(" Warning: Cached Option "
+ "%s_vlarb_%s:vl=%s"
+ " improperly formatted\n",
+ prefix, suffix, vl_str);
+ else if (vl < 0 || vl > 14)
+ log_report(" Warning: Cached Option "
+ "%s_vlarb_%s:vl=%ld out of range\n",
+ prefix, suffix, vl);
+
+ weight = strtol(weight_str, &end, 0);
+
+ if (*end)
+ log_report(" Warning: Cached Option "
+ "%s_vlarb_%s:weight=%s "
+ "improperly formatted\n",
+ prefix, suffix, weight_str);
+ else if (weight < 0 || weight > 255)
+ log_report(" Warning: Cached Option "
+ "%s_vlarb_%s:weight=%ld "
+ "out of range\n",
+ prefix, suffix, weight);
+ } else
+ log_report(" Warning: Cached Option "
+ "%s_vlarb_%s:vl:weight=%s "
+ "improperly formatted\n",
+ prefix, suffix, tok);
+
+ count++;
+ tok = strtok_r(NULL, ",\n", &ptr);
}
+
+ if (count > 64)
+ log_report(" Warning: Cached Option %s_vlarb_%s: > 64 listed:"
+ " excess vl:weight pairs will be dropped\n",
+ prefix, suffix);
+
+ free(str);
}
-static void subn_verify_sl2vl(char *sl2vl, const char *prefix)
+static void subn_verify_sl2vl(char **sl2vl, const char *prefix, char *dflt)
{
- if (sl2vl) {
- char *str, *tok, *end, *ptr;
- int count = 0;
+ char *str, *tok, *end, *ptr;
+ int count = 0;
+
+ if (*sl2vl == NULL) {
+ log_report(" Invalid Cached Option: %s_sl2vl: Using Default\n",
+ prefix);
+ *sl2vl = dflt;
+ return;
+ }
- str = strdup(sl2vl);
+ str = strdup(*sl2vl);
- tok = strtok_r(str, ",\n", &ptr);
- while (tok) {
- long vl = strtol(tok, &end, 0);
+ tok = strtok_r(str, ",\n", &ptr);
+ while (tok) {
+ long vl = strtol(tok, &end, 0);
- if (*end)
- log_report(" Warning: Cached Option %s_sl2vl:"
- "vl=%s improperly formatted\n",
- prefix, tok);
- else if (vl < 0 || vl > 15)
- log_report(" Warning: Cached Option %s_sl2vl:"
- "vl=%ld out of range\n",
- prefix, vl);
-
- count++;
- tok = strtok_r(NULL, ",\n", &ptr);
- }
+ if (*end)
+ log_report(" Warning: Cached Option %s_sl2vl:vl=%s "
+ "improperly formatted\n", prefix, tok);
+ else if (vl < 0 || vl > 15)
+ log_report(" Warning: Cached Option %s_sl2vl:vl=%ld "
+ "out of range\n", prefix, vl);
- if (count < 16)
- log_report(" Warning: Cached Option %s_sl2vl: < 16 VLs "
- "listed\n", prefix);
+ count++;
+ tok = strtok_r(NULL, ",\n", &ptr);
+ }
- if (count > 16)
- log_report(" Warning: Cached Option %s_sl2vl: "
- "> 16 listed: excess VLs will be dropped\n",
- prefix);
+ if (count < 16)
+ log_report(" Warning: Cached Option %s_sl2vl: < 16 VLs "
+ "listed\n", prefix);
- free(str);
- }
+ if (count > 16)
+ log_report(" Warning: Cached Option %s_sl2vl: > 16 listed: "
+ "excess VLs will be dropped\n", prefix);
+
+ free(str);
}
-static void subn_verify_qos_set(osm_qos_options_t *set, const char *prefix)
+static void subn_verify_qos_set(osm_qos_options_t *set, const char *prefix,
+ osm_qos_options_t *dflt)
{
- subn_verify_max_vls(&set->max_vls, prefix);
- subn_verify_high_limit(&set->high_limit, prefix);
- subn_verify_vlarb(set->vlarb_low, prefix, "low");
- subn_verify_vlarb(set->vlarb_high, prefix, "high");
- subn_verify_sl2vl(set->sl2vl, prefix);
+ subn_verify_max_vls(&set->max_vls, prefix, dflt->max_vls);
+ subn_verify_high_limit(&set->high_limit, prefix, dflt->high_limit);
+ subn_verify_vlarb(&set->vlarb_low, prefix, "low", dflt->vlarb_low);
+ subn_verify_vlarb(&set->vlarb_high, prefix, "high", dflt->vlarb_high);
+ subn_verify_sl2vl(&set->sl2vl, prefix, dflt->sl2vl);
}
static void subn_verify_conf_file(IN osm_subn_opt_t * const p_opts)
@@ -957,11 +985,24 @@ static void subn_verify_conf_file(IN osm_subn_opt_t * const p_opts)
}
if (p_opts->qos) {
- subn_verify_qos_set(&p_opts->qos_options, "qos");
- subn_verify_qos_set(&p_opts->qos_ca_options, "qos_ca");
- subn_verify_qos_set(&p_opts->qos_sw0_options, "qos_sw0");
- subn_verify_qos_set(&p_opts->qos_swe_options, "qos_swe");
- subn_verify_qos_set(&p_opts->qos_rtr_options, "qos_rtr");
+ osm_qos_options_t dflt;
+
+ /* the default options in qos_options must be correct.
+ * every other one need not be, b/c those will default
+ * back to whatever is in qos_options.
+ */
+
+ subn_set_default_qos_options(&dflt);
+
+ subn_verify_qos_set(&p_opts->qos_options, "qos", &dflt);
+ subn_verify_qos_set(&p_opts->qos_ca_options, "qos_ca",
+ &p_opts->qos_options);
+ subn_verify_qos_set(&p_opts->qos_sw0_options, "qos_sw0",
+ &p_opts->qos_options);
+ subn_verify_qos_set(&p_opts->qos_swe_options, "qos_swe",
+ &p_opts->qos_options);
+ subn_verify_qos_set(&p_opts->qos_rtr_options, "qos_rtr",
+ &p_opts->qos_options);
}
#ifdef ENABLE_OSM_PERF_MGR
@@ -1267,30 +1308,31 @@ int osm_subn_rescan_conf_files(IN osm_subn_t * const p_subn)
return -1;
}
+ subn_init_qos_options(&p_subn->opt.qos_options);
+ subn_init_qos_options(&p_subn->opt.qos_ca_options);
+ subn_init_qos_options(&p_subn->opt.qos_sw0_options);
+ subn_init_qos_options(&p_subn->opt.qos_swe_options);
+ subn_init_qos_options(&p_subn->opt.qos_rtr_options);
+
while (fgets(line, 1023, opts_file) != NULL) {
/* get the first token */
p_key = strtok_r(line, " \t\n", &p_last);
if (p_key) {
p_val = strtok_r(NULL, " \t\n", &p_last);
- subn_parse_qos_options("qos",
- p_key, p_val,
+ subn_parse_qos_options("qos", p_key, p_val,
&p_subn->opt.qos_options);
- subn_parse_qos_options("qos_ca",
- p_key, p_val,
+ subn_parse_qos_options("qos_ca", p_key, p_val,
&p_subn->opt.qos_ca_options);
- subn_parse_qos_options("qos_sw0",
- p_key, p_val,
+ subn_parse_qos_options("qos_sw0", p_key, p_val,
&p_subn->opt.qos_sw0_options);
- subn_parse_qos_options("qos_swe",
- p_key, p_val,
+ subn_parse_qos_options("qos_swe", p_key, p_val,
&p_subn->opt.qos_swe_options);
- subn_parse_qos_options("qos_rtr",
- p_key, p_val,
+ subn_parse_qos_options("qos_rtr", p_key, p_val,
&p_subn->opt.qos_rtr_options);
}
--
1.6.0.3.517.g759a
More information about the general
mailing list