Re: [PATCH v5 01/13] eal: add param register infrastructure

["Laatz, Kevin" <kevin.laatz@intel.com>]

Hi Gaetan,

Thanks for reviewing and providing suggestions.


On 17/10/2018 16:56, Gaëtan Rivet wrote:

>> +
>> +int
>> +rte_param_parse(char *option)
>> +{
>> +	/* Check if the option is in the registered inits */
>> +	TAILQ_FOREACH(param, &rte_param_list, next) {
>> +		if (strcmp(option, param->eal_option) == 0) {
>> +			param->enabled = 1;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +void __rte_experimental
>> +rte_param_register(struct rte_param *reg_param)
>> +{
>> +	TAILQ_INSERT_HEAD(&rte_param_list, reg_param, next);
> What happens when an option already exists in the list?
Will add a check to avoid option duplication, good spot. Thanks
>> @@ -1071,6 +1080,13 @@ rte_eal_init(int argc, char **argv)
>>   
>>   	rte_eal_mcfg_complete();
>>   
>> +	/* Call each registered callback, if enabled */
>> +	ret = rte_param_init();
>> +	if (ret < 0) {
>> +		rte_eal_init_alert("Callback failed\n");
> It would be better to be able to know which function failed
> and why. "Callback failed" is not helpful to the user.
> Maybe rte_option_init() return value should be expanded to allow
> for error string, error value to be passed, that could come from the
> library itself, or simply printing the option name that is the source of
> the error.
I agree that the error message is not helpful. Expanding the return 
value to pass the option name or more would be difficult however as we 
would need to check for success/fail of the execution of the callback in 
rte_option_init(). Since the we don't know what the return type of a 
given registered callback is, it is difficult to do the error checking here.
With that in mind, I think it might be best to leave the library's 
function do the error checking itself, and changing the return type of 
rte_option_init() to void. This way we could get library specific errors 
using RTE_LOG, for example. Thoughts?

Best regards,
Kevin