From: "lihuisong (C)" <lihuisong@huawei.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev <dev@dpdk.org>, "Yigit, Ferruh" <ferruh.yigit@intel.com>,
"Min Hu (Connor)" <humin29@huawei.com>
Subject: Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU
Date: Thu, 18 Nov 2021 10:44:20 +0800 [thread overview]
Message-ID: <bb9635da-ae5e-d0a6-02fa-13ee88f9419c@huawei.com> (raw)
In-Reply-To: <CAJFAV8xujpZ3s3wD8VHVEvYT4tpu9NdSrj+WSu6Gj2XMqgAsmA@mail.gmail.com>
在 2021/11/18 1:49, David Marchand 写道:
> On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
>> from 'int' to 'uint16_t'.
> You did not describe why this change is needed.
Ensure that the input parameter of rte_ethtool_net_change_mtu() API in lib/rte_ethtool.c
re consistent with rte_eth_dev_set_mtu() in ethdev layer.
I will fix commit log.
>
>
>> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> examples/ethtool/ethtool-app/ethapp.c | 6 +-----
>> examples/ethtool/lib/rte_ethtool.c | 9 +++++----
>> examples/ethtool/lib/rte_ethtool.h | 2 +-
>> 3 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index 36a1c37..fc743ce 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
>> {
>> struct pcmd_intstr_params *params = ptr_params;
>> int stat;
>> - int new_mtu;
>> + uint16_t new_mtu;
> strtoul can overflow the stack, when storing to new_mtu some lines below.
> You should either change new_mtu to unsigned long int or switch
> strtoul to some other integer format helper.
You are right. I want add an mtu specific cmd parameter structure:
struct pcmd_mtu_params {
cmdline_fixed_string_t cmd;
uint16_t port;
uint16_t mtu_value;
};
MTU size is limited when users enter the value.
Like:
cmdline_parse_token_num_t pcmd_mtu_token_port =
TOKEN_NUM_INITIALIZER(struct pcmd_mtu_params, mtu_value, RTE_UINT16);
What do you think, David?
>
>> char *ptr_parse_end;
>>
>> - if (!rte_eth_dev_is_valid_port(params->port)) {
>> - printf("Error: Invalid port number %i\n", params->port);
>> - return;
>> - }
> Adding the check in lib/rte_ethtool.c is ok, but why not keep the
> check in the application?
All right, let it stay here.
>
>
>> new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> if (*ptr_parse_end != '\0' ||
>> new_mtu < RTE_ETHER_MIN_MTU ||
> With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
> You should add a check on new_mtu < UINT16_MAX.
>
If we use above new method, this check can be removed.
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 4132516..73193ed 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>> return rte_is_valid_assigned_ether_addr(addr);
>> }
>>
>> +
>> int
>> -rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
>> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>> {
>> - if (mtu < 0 || mtu > UINT16_MAX)
>> - return -EINVAL;
>> - return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + return rte_eth_dev_set_mtu(port_id, mtu);
>> }
>>
>> int
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index f177096..fe3250e 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>> * - (-EINVAL) if parameters invalid.
>> * - others depends on the specific operations implementation.
>> */
>> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
>> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>>
>> /**
>> * Retrieve the Ethernet device traffic statistics
>> --
>> 2.8.1
>>
>
next prev parent reply other threads:[~2021-11-18 2:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-29 10:53 [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
2021-11-17 17:49 ` David Marchand
2021-11-18 2:44 ` lihuisong (C) [this message]
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query " Min Hu (Connor)
2021-11-17 17:51 ` David Marchand
2021-11-18 2:58 ` lihuisong (C)
2021-06-28 3:23 ` [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2023-07-03 21:37 ` Stephen Hemminger
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=bb9635da-ae5e-d0a6-02fa-13ee88f9419c@huawei.com \
--to=lihuisong@huawei.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=humin29@huawei.com \
/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).