From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: "Ajmera, Megha" <megha.ajmera@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Singh, Jasvinder" <jasvinder.singh@intel.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: RE: [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe config
Date: Mon, 24 Oct 2022 18:57:40 +0000 [thread overview]
Message-ID: <CH0PR11MB57242F5F398B26ED07C188DCEB2E9@CH0PR11MB5724.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20221020094747.605087-3-megha.ajmera@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
next prev parent reply other threads:[~2022-10-24 18:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-05 17:22 [PATCH v2] sched: Fix subport profile id not set correctly Megha Ajmera
2022-10-05 19:19 ` Dumitrescu, Cristian
2022-10-11 4:59 ` Ajmera, Megha
2022-10-06 19:00 ` [PATCH 1/3] sched: fix " Megha Ajmera
2022-10-06 19:00 ` [PATCH 2/3] sched: removed unused subport field in hqos profile Megha Ajmera
2022-10-11 14:26 ` Dumitrescu, Cristian
2022-10-06 19:00 ` [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config Megha Ajmera
2022-10-11 13:09 ` Dumitrescu, Cristian
2022-10-18 5:40 ` Ajmera, Megha
2022-10-18 13:12 ` Dumitrescu, Cristian
2022-10-19 18:37 ` Stephen Hemminger
2022-10-20 9:47 ` [PATCH v4 1/3] sched: fix subport profile ID Megha Ajmera
2022-10-20 9:47 ` [PATCH v4 2/3] sched: fix number of subport profiles Megha Ajmera
2022-10-20 9:47 ` [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe config Megha Ajmera
2022-10-24 18:57 ` Dumitrescu, Cristian [this message]
2022-10-20 9:55 ` [PATCH " Ajmera, Megha
2022-10-11 14:22 ` [PATCH 1/3] sched: fix subport profile id not set correctly Dumitrescu, Cristian
2022-10-28 8:09 ` [PATCH v5 1/3] sched: fix subport profile ID Megha Ajmera
2022-10-28 8:09 ` [PATCH v5 2/3] sched: fix number of subport profiles Megha Ajmera
2022-10-28 8:09 ` [PATCH v5 3/3] sched: support for 100G+ rates in subport/pipe config Megha Ajmera
2022-10-28 9:55 ` [PATCH v6 1/3] sched: fix subport profile ID Megha Ajmera
2022-10-28 9:55 ` [PATCH v6 2/3] sched: fix number of subport profiles Megha Ajmera
2022-10-28 14:02 ` David Marchand
2022-10-28 9:55 ` [PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe config Megha Ajmera
2022-10-28 10:57 ` Dumitrescu, Cristian
2022-10-28 14:04 ` David Marchand
2022-10-28 14:01 ` [PATCH v6 1/3] sched: fix subport profile ID David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CH0PR11MB57242F5F398B26ED07C188DCEB2E9@CH0PR11MB5724.namprd11.prod.outlook.com \
--to=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=jasvinder.singh@intel.com \
--cc=megha.ajmera@intel.com \
--cc=stable@dpdk.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).