[PATCH v2 opensm] Try default partition config if parsing partitions.conf fails

[PATCH v2 opensm] Try default partition config if parsing partitions.conf fails

[Hal Rosenstock]

From: Bernd Schubert <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>

If partitions.conf is for some reason invalid or empty, try again
with the default configuration.

This will re-use the default configuration created by prtn_make_default(),
but osm_prtn_make_new() will automatically overwrite the initial default.

Signed-off-by: Bernd Schubert <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
Change from v1:
Based on Berd's original patch for this, this modifies
osm_prtn_config_parse_file determination of is_parse_success
to handle more cases than original proposed patch.

diff --git a/opensm/osm_prtn.c b/opensm/osm_prtn.c
index e76e2e1..bd0ac67 100644
--- a/opensm/osm_prtn.c
+++ b/opensm/osm_prtn.c
@@ -376,6 +376,7 @@ ib_api_status_t osm_prtn_make_partitions(osm_log_t * p_log, osm_subn_t * p_subn)
 	struct stat statbuf;
 	const char *file_name;
 	boolean_t is_config = TRUE;
+	boolean_t is_wrong_config = FALSE;
 	ib_api_status_t status = IB_SUCCESS;
 	cl_map_item_t *p_next;
 	osm_prtn_t *p;
@@ -389,6 +390,7 @@ ib_api_status_t osm_prtn_make_partitions(osm_log_t * p_log, osm_subn_t * p_subn)
 		is_config = FALSE;
 	}
 
+retry_default:
 	/* clean up current port maps */
 	p_next = cl_qmap_head(&p_subn->prtn_pkey_tbl);
 	while (p_next != cl_qmap_end(&p_subn->prtn_pkey_tbl)) {
@@ -404,9 +406,11 @@ ib_api_status_t osm_prtn_make_partitions(osm_log_t * p_log, osm_subn_t * p_subn)
 	if (status != IB_SUCCESS)
 		goto _err;
 
-	if (is_config && osm_prtn_config_parse_file(p_log, p_subn, file_name))
+	if (is_config && osm_prtn_config_parse_file(p_log, p_subn, file_name)) {
 		OSM_LOG(p_log, OSM_LOG_VERBOSE, "Partition configuration "
 			"was not fully processed\n");
+		is_wrong_config = TRUE;
+	}
 
 	/* and now clean up empty partitions */
 	p_next = cl_qmap_head(&p_subn->prtn_pkey_tbl);
@@ -421,6 +425,13 @@ ib_api_status_t osm_prtn_make_partitions(osm_log_t * p_log, osm_subn_t * p_subn)
 		}
 	}
 
+	if (is_config && is_wrong_config) {
+		OSM_LOG(p_log, OSM_LOG_ERROR, "Partition configuration "
+			"in error; retrying with default config\n");
+		is_config = FALSE;
+		goto retry_default;
+	}
+
 _err:
 	return status;
 }
diff --git a/opensm/osm_prtn_config.c b/opensm/osm_prtn_config.c
index 8f4a673..9bad7a7 100644
--- a/opensm/osm_prtn_config.c
+++ b/opensm/osm_prtn_config.c
@@ -696,6 +696,9 @@ done:
 	return len;
 }
 
+/**
+ * @return -1 on error, 0 on success
+ */
 int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 			       const char *file_name)
 {
@@ -703,6 +706,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 	struct part_conf *conf = NULL;
 	FILE *file;
 	int lineno;
+	boolean_t is_parse_success;
 
 	file = fopen(file_name, "r");
 	if (!file) {
@@ -714,6 +718,8 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 
 	lineno = 0;
 
+	is_parse_success = FALSE;
+
 	while (fgets(line, sizeof(line) - 1, file) != NULL) {
 		char *q, *p = line;
 
@@ -724,6 +730,8 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 		q = strchr(p, '#');
 		if (q)
 			*q = '\0';
+		else
+			is_parse_success = TRUE;
 
 		do {
 			int len;
@@ -741,6 +749,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 					"PARSE ERROR: line %d: "
 					"internal: cannot create config\n",
 					lineno);
+				is_parse_success = FALSE;
 				break;
 			}
 
@@ -750,6 +759,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 
 			len = parse_part_conf(conf, p, lineno);
 			if (len < 0) {
+				is_parse_success = FALSE;
 				break;
 			}
 
@@ -764,5 +774,8 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 
 	fclose(file);
 
-	return 0;
+	if (is_parse_success)
+		return 0;
+	else
+		return -1;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH opensm] Parse the the partition config in dry-run mode first

[Bernd Schubert]

Hello Hal,

thanks for updating the patch! I had it on my todo list during my vacation 
(still one week left) and didn't have the time to care about it before.

What do you think about the patch below on top of your updated patch?
(So far I only tested if it compiles at all.)


From: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>

When the partition config file is parsed, partitions are created
for each line. If one of the config lines has an error a default
partition is created already by a previous commit, but partitions
from previous lines are not cleaned up. 
Avoid this by doing a dry-run parse first and only if this succeeds 
actually create new partitions. 
This still ignores errors from osm_prtn_make_new(), but these
errors are rather unlikely (i.e. malloc fails) and not really
related to the config file.

Signed-off-by: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>
---
 opensm/osm_prtn_config.c |   47 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/opensm/osm_prtn_config.c b/opensm/osm_prtn_config.c
index 9bad7a7..4a2eaab 100644
--- a/opensm/osm_prtn_config.c
+++ b/opensm/osm_prtn_config.c
@@ -212,7 +212,8 @@ static void __create_mgrp(struct part_conf *conf, struct precreate_mgroup *group
 }
 
 static int partition_create(unsigned lineno, struct part_conf *conf,
-			    char *name, char *id, char *flag, char *flag_val)
+			    char *name, char *id, char *flag, char *flag_val,
+			    boolean_t is_dry_run)
 {
 	ib_net16_t pkey;
 
@@ -230,6 +231,9 @@ static int partition_create(unsigned lineno, struct part_conf *conf,
 	} else
 		pkey = 0;
 
+	if (is_dry_run)
+		return 0;
+
 	conf->p_prtn = osm_prtn_make_new(conf->p_log, conf->p_subn,
 					 name, pkey);
 	if (!conf->p_prtn)
@@ -601,7 +605,11 @@ static int flush_part_conf(struct part_conf *conf)
 	return 0;
 }
 
-static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
+/**
+ *@is_dry_run only test if this line is valid
+ */
+static int parse_part_conf(struct part_conf *conf, char *str, int lineno,
+			   boolean_t is_dry_run)
 {
 	int ret, len = 0;
 	char *name, *id, *flag, *flval;
@@ -659,7 +667,8 @@ static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
 	}
 
 	if (p != str || (partition_create(lineno, conf,
-					  name, id, flag, flval) < 0)) {
+					  name, id, flag, flval,
+					  is_dry_run) < 0)) {
 		OSM_LOG(conf->p_log, OSM_LOG_ERROR, "PARSE ERROR: line %d: "
 			"bad partition definition\n", lineno);
 		fprintf(stderr, "\nPARSE ERROR: line %d: "
@@ -697,10 +706,12 @@ done:
 }
 
 /**
+ * @is_dry_run test if the config is valid only
  * @return -1 on error, 0 on success
  */
-int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
-			       const char *file_name)
+static int _osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
+				       const char *file_name,
+				       boolean_t is_dry_run)
 {
 	char line[4096];
 	struct part_conf *conf = NULL;
@@ -757,7 +768,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 			if (q)
 				*q = '\0';
 
-			len = parse_part_conf(conf, p, lineno);
+			len = parse_part_conf(conf, p, lineno, is_dry_run);
 			if (len < 0) {
 				is_parse_success = FALSE;
 				break;
@@ -779,3 +790,27 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
 	else
 		return -1;
 }
+
+
+/**
+ * @return -1 on error, 0 on success
+ */
+int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
+			       const char *file_name)
+{
+	boolean_t is_dry_run = TRUE; // dry-run first
+	int parse_res;
+
+	while (1) {
+		parse_res =_osm_prtn_config_parse_file(p_log, p_subn, file_name,
+						       is_dry_run);
+		if (parse_res != 0);
+			return -1;
+
+		if (!is_dry_run)
+			return 0; // parse / partition create success
+
+		is_dry_run = FALSE;
+	}
+
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH opensm] Parse the the partition config in dry-run mode first

[Hal Rosenstock]

Hi Bernd,

On 1/4/2014 5:23 PM, Bernd Schubert wrote:
> Hello Hal,
> 
> thanks for updating the patch! I had it on my todo list during my vacation 
> (still one week left) and didn't have the time to care about it before.
> 
> What do you think about the patch below on top of your updated patch?
> (So far I only tested if it compiles at all.)
> 
> 
> From: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>
> 
> When the partition config file is parsed, partitions are created
> for each line. If one of the config lines has an error a default
> partition is created already by a previous commit, but partitions
> from previous lines are not cleaned up. 

Would a better policy for this be to honor all good partition config
lines and ignore/warn about the bad ones rather than revert to default ?

So as long as there was at least one good line in partition config it
would use that; otherwise the default partition config.

Does that make sense ?

-- Hal

> Avoid this by doing a dry-run parse first and only if this succeeds 
> actually create new partitions. 
> This still ignores errors from osm_prtn_make_new(), but these
> errors are rather unlikely (i.e. malloc fails) and not really
> related to the config file.
> 
> Signed-off-by: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>
> ---
>  opensm/osm_prtn_config.c |   47 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/opensm/osm_prtn_config.c b/opensm/osm_prtn_config.c
> index 9bad7a7..4a2eaab 100644
> --- a/opensm/osm_prtn_config.c
> +++ b/opensm/osm_prtn_config.c
> @@ -212,7 +212,8 @@ static void __create_mgrp(struct part_conf *conf, struct precreate_mgroup *group
>  }
>  
>  static int partition_create(unsigned lineno, struct part_conf *conf,
> -			    char *name, char *id, char *flag, char *flag_val)
> +			    char *name, char *id, char *flag, char *flag_val,
> +			    boolean_t is_dry_run)
>  {
>  	ib_net16_t pkey;
>  
> @@ -230,6 +231,9 @@ static int partition_create(unsigned lineno, struct part_conf *conf,
>  	} else
>  		pkey = 0;
>  
> +	if (is_dry_run)
> +		return 0;
> +
>  	conf->p_prtn = osm_prtn_make_new(conf->p_log, conf->p_subn,
>  					 name, pkey);
>  	if (!conf->p_prtn)
> @@ -601,7 +605,11 @@ static int flush_part_conf(struct part_conf *conf)
>  	return 0;
>  }
>  
> -static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
> +/**
> + *@is_dry_run only test if this line is valid
> + */
> +static int parse_part_conf(struct part_conf *conf, char *str, int lineno,
> +			   boolean_t is_dry_run)
>  {
>  	int ret, len = 0;
>  	char *name, *id, *flag, *flval;
> @@ -659,7 +667,8 @@ static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
>  	}
>  
>  	if (p != str || (partition_create(lineno, conf,
> -					  name, id, flag, flval) < 0)) {
> +					  name, id, flag, flval,
> +					  is_dry_run) < 0)) {
>  		OSM_LOG(conf->p_log, OSM_LOG_ERROR, "PARSE ERROR: line %d: "
>  			"bad partition definition\n", lineno);
>  		fprintf(stderr, "\nPARSE ERROR: line %d: "
> @@ -697,10 +706,12 @@ done:
>  }
>  
>  /**
> + * @is_dry_run test if the config is valid only
>   * @return -1 on error, 0 on success
>   */
> -int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
> -			       const char *file_name)
> +static int _osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
> +				       const char *file_name,
> +				       boolean_t is_dry_run)
>  {
>  	char line[4096];
>  	struct part_conf *conf = NULL;
> @@ -757,7 +768,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>  			if (q)
>  				*q = '\0';
>  
> -			len = parse_part_conf(conf, p, lineno);
> +			len = parse_part_conf(conf, p, lineno, is_dry_run);
>  			if (len < 0) {
>  				is_parse_success = FALSE;
>  				break;
> @@ -779,3 +790,27 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>  	else
>  		return -1;
>  }
> +
> +
> +/**
> + * @return -1 on error, 0 on success
> + */
> +int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
> +			       const char *file_name)
> +{
> +	boolean_t is_dry_run = TRUE; // dry-run first
> +	int parse_res;
> +
> +	while (1) {
> +		parse_res =_osm_prtn_config_parse_file(p_log, p_subn, file_name,
> +						       is_dry_run);
> +		if (parse_res != 0);
> +			return -1;
> +
> +		if (!is_dry_run)
> +			return 0; // parse / partition create success
> +
> +		is_dry_run = FALSE;
> +	}
> +
> +}
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH opensm] Parse the the partition config in dry-run mode first

[Hal Rosenstock]

On 1/6/2014 11:35 AM, Hal Rosenstock wrote:
> Hi Bernd,
> 
> On 1/4/2014 5:23 PM, Bernd Schubert wrote:
>> Hello Hal,
>>
>> thanks for updating the patch! I had it on my todo list during my vacation 
>> (still one week left) and didn't have the time to care about it before.
>>
>> What do you think about the patch below on top of your updated patch?
>> (So far I only tested if it compiles at all.)
>>
>>
>> From: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>
>>
>> When the partition config file is parsed, partitions are created
>> for each line. If one of the config lines has an error a default
>> partition is created already by a previous commit, but partitions
>> from previous lines are not cleaned up. 
> 
> Would a better policy for this be to honor all good partition config
> lines and ignore/warn about the bad ones rather than revert to default ?
> 
> So as long as there was at least one good line in partition config it
> would use that; otherwise the default partition config.
> 
> Does that make sense ?

Current policy is to take the part of the configuration that is correct
and ignore the errors (logging the line numbers on which they occur).
This seems to be slightly better than defaulting back to the default
partition config (just default partition with IPoIB) as it is likely to
be closer to what the admin wants as a policy so I'd prefer to leave it
this way unless there's some stronger motivation to change this.

-- Hal

> 
> -- Hal
> 
>> Avoid this by doing a dry-run parse first and only if this succeeds 
>> actually create new partitions. 
>> This still ignores errors from osm_prtn_make_new(), but these
>> errors are rather unlikely (i.e. malloc fails) and not really
>> related to the config file.
>>
>> Signed-off-by: Bernd Schubert <bernd.schubert-97jfqw80gc6171pxa8y+qA@public.gmane.org>
>> ---
>>  opensm/osm_prtn_config.c |   47 ++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/opensm/osm_prtn_config.c b/opensm/osm_prtn_config.c
>> index 9bad7a7..4a2eaab 100644
>> --- a/opensm/osm_prtn_config.c
>> +++ b/opensm/osm_prtn_config.c
>> @@ -212,7 +212,8 @@ static void __create_mgrp(struct part_conf *conf, struct precreate_mgroup *group
>>  }
>>  
>>  static int partition_create(unsigned lineno, struct part_conf *conf,
>> -			    char *name, char *id, char *flag, char *flag_val)
>> +			    char *name, char *id, char *flag, char *flag_val,
>> +			    boolean_t is_dry_run)
>>  {
>>  	ib_net16_t pkey;
>>  
>> @@ -230,6 +231,9 @@ static int partition_create(unsigned lineno, struct part_conf *conf,
>>  	} else
>>  		pkey = 0;
>>  
>> +	if (is_dry_run)
>> +		return 0;
>> +
>>  	conf->p_prtn = osm_prtn_make_new(conf->p_log, conf->p_subn,
>>  					 name, pkey);
>>  	if (!conf->p_prtn)
>> @@ -601,7 +605,11 @@ static int flush_part_conf(struct part_conf *conf)
>>  	return 0;
>>  }
>>  
>> -static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
>> +/**
>> + *@is_dry_run only test if this line is valid
>> + */
>> +static int parse_part_conf(struct part_conf *conf, char *str, int lineno,
>> +			   boolean_t is_dry_run)
>>  {
>>  	int ret, len = 0;
>>  	char *name, *id, *flag, *flval;
>> @@ -659,7 +667,8 @@ static int parse_part_conf(struct part_conf *conf, char *str, int lineno)
>>  	}
>>  
>>  	if (p != str || (partition_create(lineno, conf,
>> -					  name, id, flag, flval) < 0)) {
>> +					  name, id, flag, flval,
>> +					  is_dry_run) < 0)) {
>>  		OSM_LOG(conf->p_log, OSM_LOG_ERROR, "PARSE ERROR: line %d: "
>>  			"bad partition definition\n", lineno);
>>  		fprintf(stderr, "\nPARSE ERROR: line %d: "
>> @@ -697,10 +706,12 @@ done:
>>  }
>>  
>>  /**
>> + * @is_dry_run test if the config is valid only
>>   * @return -1 on error, 0 on success
>>   */
>> -int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>> -			       const char *file_name)
>> +static int _osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>> +				       const char *file_name,
>> +				       boolean_t is_dry_run)
>>  {
>>  	char line[4096];
>>  	struct part_conf *conf = NULL;
>> @@ -757,7 +768,7 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>>  			if (q)
>>  				*q = '\0';
>>  
>> -			len = parse_part_conf(conf, p, lineno);
>> +			len = parse_part_conf(conf, p, lineno, is_dry_run);
>>  			if (len < 0) {
>>  				is_parse_success = FALSE;
>>  				break;
>> @@ -779,3 +790,27 @@ int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>>  	else
>>  		return -1;
>>  }
>> +
>> +
>> +/**
>> + * @return -1 on error, 0 on success
>> + */
>> +int osm_prtn_config_parse_file(osm_log_t * p_log, osm_subn_t * p_subn,
>> +			       const char *file_name)
>> +{
>> +	boolean_t is_dry_run = TRUE; // dry-run first
>> +	int parse_res;
>> +
>> +	while (1) {
>> +		parse_res =_osm_prtn_config_parse_file(p_log, p_subn, file_name,
>> +						       is_dry_run);
>> +		if (parse_res != 0);
>> +			return -1;
>> +
>> +		if (!is_dry_run)
>> +			return 0; // parse / partition create success
>> +
>> +		is_dry_run = FALSE;
>> +	}
>> +
>> +}
>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html