[ofa-general] Re: [PATCH 7/7 V3] osm: QoS - reading policy file

[Yevgeny Kliteynik]

Sasha Khapyorsky wrote:
> On 02:11 Tue 28 Aug     , Yevgeny Kliteynik wrote:
>> Reading QoS policy file
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
> 
> Applied. Thanks.
> 
> Some comments are below.
> 
>> ---
>>  opensm/include/opensm/osm_subnet.h |   16 ++++++++--------
>>  opensm/opensm/osm_state_mgr.c      |    2 +-
>>  opensm/opensm/osm_subnet.c         |   17 +++++++++++------
>>  3 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
>> index 5e8b522..7e1a3e7 100644
>> --- a/opensm/include/opensm/osm_subnet.h
>> +++ b/opensm/include/opensm/osm_subnet.h
>> @@ -1118,28 +1118,28 @@ ib_api_status_t osm_subn_parse_conf_file(IN osm_subn_opt_t * const p_opt);
>>  *	Subnet object, osm_subn_construct, osm_subn_destroy
>>  *********/
>>
>> -/****f* OpenSM: Subnet/osm_subn_parse_conf_file
>> +/****f* OpenSM: Subnet/osm_subn_parse_conf_files
>>  * NAME
>> -*	osm_subn_rescan_conf_file
>> +*	osm_subn_rescan_conf_files
>>  *
>>  * DESCRIPTION
>> -*	The osm_subn_rescan_conf_file function parses the configuration
>> -*	file and update selected subnet options
>> +*	The osm_subn_rescan_conf_files function parses the configuration
>> +*	files and update selected subnet options
>>  *
>>  * SYNOPSIS
>>  */
>> -ib_api_status_t osm_subn_rescan_conf_file(IN osm_subn_opt_t * const p_opts);
>> +ib_api_status_t osm_subn_rescan_conf_files(IN osm_subn_t * const p_subn);
>>  /*
>>  * PARAMETERS
>>  *
>> -*	p_opt
>> -*		[in] Pointer to the subnet options structure.
>> +*	p_subn
>> +*		[in] Pointer to the subnet structure.
>>  *
>>  * RETURN VALUES
>>  *	IB_SUCCESS, IB_ERROR
>>  *
>>  * NOTES
>> -*  This uses the same file as osm_subn_parse_conf_file()
>> +*  This uses the same file as osm_subn_parse_conf_files()
>>  *
>>  *********/
>>
>> diff --git a/opensm/opensm/osm_state_mgr.c b/opensm/opensm/osm_state_mgr.c
>> index 030d344..2a94518 100644
>> --- a/opensm/opensm/osm_state_mgr.c
>> +++ b/opensm/opensm/osm_state_mgr.c
>> @@ -1878,7 +1878,7 @@ void osm_state_mgr_process(IN osm_state_mgr_t * const p_mgr,
>>  					p_mgr->p_subn->subnet_initialization_error = FALSE;
>>
>>  					/* rescan configuration updates */
>> -					status = osm_subn_rescan_conf_file(&p_mgr->p_subn->opt);
>> +					status = osm_subn_rescan_conf_files(p_mgr->p_subn);
>>  					if (status != IB_SUCCESS) {
>>  						osm_log(p_mgr->p_log,
>>  							OSM_LOG_ERROR,
>> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
>> index 4162522..c847daf 100644
>> --- a/opensm/opensm/osm_subnet.c
>> +++ b/opensm/opensm/osm_subnet.c
>> @@ -69,6 +69,7 @@
>>  #include <opensm/osm_console.h>
>>  #include <opensm/osm_perfmgr.h>
>>  #include <opensm/osm_event_plugin.h>
>> +#include <opensm/osm_qos_policy.h>
>>
>>  #if defined(PATH_MAX)
>>  #define OSM_PATH_MAX	(PATH_MAX + 1)
>> @@ -677,7 +678,7 @@ subn_dump_qos_options(FILE * file,
>>
>>  /**********************************************************************
>>   **********************************************************************/
>> -ib_api_status_t osm_subn_rescan_conf_file(IN osm_subn_opt_t * const p_opts)
>> +ib_api_status_t osm_subn_rescan_conf_files(IN osm_subn_t * const p_subn)
>>  {
>>  	char *p_cache_dir = getenv("OSM_CACHE_DIR");
>>  	char file_name[OSM_PATH_MAX];
>> @@ -704,28 +705,32 @@ ib_api_status_t osm_subn_rescan_conf_file(IN osm_subn_opt_t * const p_opts)
>>
>>  			subn_parse_qos_options("qos",
>>  					       p_key, p_val,
>> -					       &p_opts->qos_options);
>> +					       &p_subn->opt.qos_options);
>>
>>  			subn_parse_qos_options("qos_ca",
>>  					       p_key, p_val,
>> -					       &p_opts->qos_ca_options);
>> +					       &p_subn->opt.qos_ca_options);
>>
>>  			subn_parse_qos_options("qos_sw0",
>>  					       p_key, p_val,
>> -					       &p_opts->qos_sw0_options);
>> +					       &p_subn->opt.qos_sw0_options);
>>
>>  			subn_parse_qos_options("qos_swe",
>>  					       p_key, p_val,
>> -					       &p_opts->qos_swe_options);
>> +					       &p_subn->opt.qos_swe_options);
>>
>>  			subn_parse_qos_options("qos_rtr",
>>  					       p_key, p_val,
>> -					       &p_opts->qos_rtr_options);
>> +					       &p_subn->opt.qos_rtr_options);
>>
>>  		}
>>  	}
>>  	fclose(opts_file);
>>
>> +	/* read QoS policy config file */
>> +	if (!p_subn->opt.no_qos)
>> +		osm_qos_parse_policy_file(p_subn);
>> +
> 
> Should the qos parser return value be checked and an error logged in
> case of failure?

The parser logs the errors (whether the policy file is not found, or there is
a syntax error, or any other error).
If some error occurred, the p_subn->p_qos_policy will be null.
Do you think I should add status checking here as well?
I think it would be redundant.

-- Yevgeny

> Sasha
> 
>>  	return IB_SUCCESS;
>>  }
>>
>> -- 
>> 1.5.1.4
>>
>