[dpdk-dev] [PATCH v5 01/13] eal: add param register infrastructure
Laatz, Kevin
kevin.laatz at intel.com
Thu Oct 18 17:58:28 CEST 2018
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
More information about the dev
mailing list