From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id BD9351B120 for ; Thu, 18 Oct 2018 17:58:32 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Oct 2018 08:58:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,396,1534834800"; d="scan'208";a="100578047" Received: from klaatz-mobl.ger.corp.intel.com (HELO [10.237.220.107]) ([10.237.220.107]) by orsmga001.jf.intel.com with ESMTP; 18 Oct 2018 08:58:29 -0700 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Cc: dev@dpdk.org, harry.van.haaren@intel.com, stephen@networkplumber.org, shreyansh.jain@nxp.com, thomas@monjalon.net, mattias.ronnblom@ericsson.com, bruce.richardson@intel.com References: <20181011165837.81030-1-kevin.laatz@intel.com> <20181016155802.2067-1-kevin.laatz@intel.com> <20181016155802.2067-2-kevin.laatz@intel.com> <20181017155602.kz2kpxx76cwutnbg@bidouze.vm.6wind.com> From: "Laatz, Kevin" Message-ID: Date: Thu, 18 Oct 2018 16:58:28 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181017155602.kz2kpxx76cwutnbg@bidouze.vm.6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v5 01/13] eal: add param register infrastructure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Oct 2018 15:58:33 -0000 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