RE: [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe config

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Megha,

Comments inline below.

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Thursday, October 20, 2022 10:48 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> stephen@networkplumber.org
> Cc: stable@dpdk.org
> Subject: [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe
> config
> 
> Config load functions updated to support 100G rates
> for subport and pipes.
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/cfg_file.c | 294 ++++++++++++++++++++++++++------
> --
>  1 file changed, 230 insertions(+), 64 deletions(-)
> 
> diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
> index ca871d3287..9fcdf5eeb6 100644
> --- a/examples/qos_sched/cfg_file.c
> +++ b/examples/qos_sched/cfg_file.c
> @@ -60,71 +60,154 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct
> rte_sched_pipe_params *pipe_params
> 
>  	for (j = 0; j < profiles; j++) {
>  		char pipe_name[32];
> +		/* Convert to decimal */
> +		int base = 10;

We should set base to 0 to allow multiple basis (8, 10, 16), I see no reason to limit to base 10, let's take the benefits out of strtoull. I suggest we use the value 0 directly in the function, no need to have a local variable just for this.
> +
>  		snprintf(pipe_name, sizeof(pipe_name), "pipe profile %d",
> j);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
> -		if (entry)
> -			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
> +		if (entry) {
> +			pipe_params[j].tb_rate = (uint64_t)strtoull(entry,
> NULL, base);
> +			if (errno == EINVAL || errno == ERANGE) {
> +				/* cannot convert string to unsigned long
> long */
> +				return -1;
> +			}
> +		}

Some comments for this recurring pattern:

1. Why are you still doing conversion to uint64_t, as strtoull already returns a 64-bit unsigned integer?
2. If you are checking errno after the call, you should first set errno to 0 before the call, right? See man strtoull, please.
3. The errno check does not fully handle the error cases, you should also use the second argument (as opposed to setting it to NULL) to check that after the call the value referenced by the second argument is 0. This ensures that there are no illegal characters after the digits, such as in the case of "1234qwerty".
4. The comment just before the return does not bring any value, as it is obvious that an error took place if you're returning an error.
5. I suggest you wrap the strtoull() usage into local function, such as parse_u64() or similar.


<snip>

Regards,
Cristian