[05/12] net/hns3: fix an unreasonable memset
Checks
Commit Message
From: Huisong Li <lihuisong@huawei.com>
This patch fixes an unreasonable memset.
Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
drivers/net/hns3/hns3_rxtx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 5/19/22 15:29, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> This patch fixes an unreasonable memset.
>
> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> drivers/net/hns3/hns3_rxtx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 510802be05..5a2cfe5a54 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
> int ret;
> uint16_t i;
>
> - memset(msg_data, 0, sizeof(uint16_t));
> + memset(msg_data, 0, sizeof(msg_data));
It looks a bit suspicious. May be it is better to do:
memset(&msg_data, 0, sizeof(msg_data));
It is pretty common mistake to do:
memset(p, 0, sizeof(p));
instead of
memset(p, 0, sizeof(*p));
when p is a pointer.
I realize that msg_data is an array in this case, but
I think it is better to avoid bad pattern in the code.
> ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
> sizeof(msg_data), true, &reset_status,
> sizeof(reset_status));
Hi, Andrew ,
在 2022/5/20 15:58, Andrew Rybchenko 写道:
> On 5/19/22 15:29, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch fixes an unreasonable memset.
>>
>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>> index 510802be05..5a2cfe5a54 100644
>> --- a/drivers/net/hns3/hns3_rxtx.c
>> +++ b/drivers/net/hns3/hns3_rxtx.c
>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>> int ret;
>> uint16_t i;
>> - memset(msg_data, 0, sizeof(uint16_t));
>> + memset(msg_data, 0, sizeof(msg_data));
>
> It looks a bit suspicious. May be it is better to do:
> memset(&msg_data, 0, sizeof(msg_data));
I think this is too hard to understand for &msg_data is **p.
maybe it confuse others when use memset to operate level-2 pointer.
> It is pretty common mistake to do:
> memset(p, 0, sizeof(p));
> instead of
> memset(p, 0, sizeof(*p));
> when p is a pointer.
> I realize that msg_data is an array in this case, but
> I think it is better to avoid bad pattern in the code.
>
>> ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>> sizeof(msg_data), true, &reset_status,
>> sizeof(reset_status));
>
> .
On 5/20/22 11:37, Min Hu (Connor) wrote:
> Hi, Andrew ,
>
> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> This patch fixes an unreasonable memset.
>>>
>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>>> index 510802be05..5a2cfe5a54 100644
>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>> int ret;
>>> uint16_t i;
>>> - memset(msg_data, 0, sizeof(uint16_t));
>>> + memset(msg_data, 0, sizeof(msg_data));
>>
>> It looks a bit suspicious. May be it is better to do:
>> memset(&msg_data, 0, sizeof(msg_data));
> I think this is too hard to understand for &msg_data is **p.
> maybe it confuse others when use memset to operate level-2 pointer.
msg_data == &amsg_data
since it is an array
>
>
>> It is pretty common mistake to do:
>> memset(p, 0, sizeof(p));
>> instead of
>> memset(p, 0, sizeof(*p));
>> when p is a pointer.
>> I realize that msg_data is an array in this case, but
>> I think it is better to avoid bad pattern in the code.
>>
>>> ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>>> sizeof(msg_data), true, &reset_status,
>>> sizeof(reset_status));
>>
>> .
Hi,Andrew,
在 2022/5/20 16:59, Andrew Rybchenko 写道:
> On 5/20/22 11:37, Min Hu (Connor) wrote:
>> Hi, Andrew ,
>>
>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> This patch fixes an unreasonable memset.
>>>>
>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c
>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>> index 510802be05..5a2cfe5a54 100644
>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>> int ret;
>>>> uint16_t i;
>>>> - memset(msg_data, 0, sizeof(uint16_t));
>>>> + memset(msg_data, 0, sizeof(msg_data));
>>>
>>> It looks a bit suspicious. May be it is better to do:
>>> memset(&msg_data, 0, sizeof(msg_data));
>> I think this is too hard to understand for &msg_data is **p.
>> maybe it confuse others when use memset to operate level-2 pointer.
>
> msg_data == &amsg_data
> since it is an array
Yes, you are right, the address of 'msg_data ' is the same
as the address of '&amsg_data'.
But I still think this usage is not common.
like the current code in DPDK:
*****
static int
app_parse_flow_conf(const char *conf_str)
{
int ret;
uint32_t vals[5];
struct flow_conf *pconf;
uint64_t mask;
memset(vals, 0, sizeof(vals));
...
*****
how about others's opinion? @Ferruh, @Thomas.
>
>>
>>
>>> It is pretty common mistake to do:
>>> memset(p, 0, sizeof(p));
>>> instead of
>>> memset(p, 0, sizeof(*p));
>>> when p is a pointer.
>>> I realize that msg_data is an array in this case, but
>>> I think it is better to avoid bad pattern in the code.
>>>
>>>> ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>>>> sizeof(msg_data), true, &reset_status,
>>>> sizeof(reset_status));
>>>
>>> .
>
> .
On 5/20/2022 10:14 AM, Min Hu (Connor) wrote:
> [CAUTION: External Email]
>
> Hi,Andrew,
>
> 在 2022/5/20 16:59, Andrew Rybchenko 写道:
>> On 5/20/22 11:37, Min Hu (Connor) wrote:
>>> Hi, Andrew ,
>>>
>>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>
>>>>> This patch fixes an unreasonable memset.
>>>>>
>>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> ---
>>>>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c
>>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>>> index 510802be05..5a2cfe5a54 100644
>>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>>> int ret;
>>>>> uint16_t i;
>>>>> - memset(msg_data, 0, sizeof(uint16_t));
>>>>> + memset(msg_data, 0, sizeof(msg_data));
>>>>
>>>> It looks a bit suspicious. May be it is better to do:
>>>> memset(&msg_data, 0, sizeof(msg_data));
>>> I think this is too hard to understand for &msg_data is **p.
>>> maybe it confuse others when use memset to operate level-2 pointer.
>>
>> msg_data == &amsg_data
>> since it is an array
> Yes, you are right, the address of 'msg_data ' is the same
> as the address of '&amsg_data'.
>
> But I still think this usage is not common.
> like the current code in DPDK:
> *****
> static int
> app_parse_flow_conf(const char *conf_str)
> {
> int ret;
> uint32_t vals[5];
> struct flow_conf *pconf;
> uint64_t mask;
>
> memset(vals, 0, sizeof(vals));
> ...
> *****
>
> how about others's opinion? @Ferruh, @Thomas.
>
I would use array without address operator, so original patch lgtm.
+1 to main purpose of the patch (to replace 'sizeof(uint16_t)').
Hi,
在 2022/5/20 18:43, Ferruh Yigit 写道:
> On 5/20/2022 10:14 AM, Min Hu (Connor) wrote:
>> [CAUTION: External Email]
>>
>> Hi,Andrew,
>>
>> 在 2022/5/20 16:59, Andrew Rybchenko 写道:
>>> On 5/20/22 11:37, Min Hu (Connor) wrote:
>>>> Hi, Andrew ,
>>>>
>>>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> This patch fixes an unreasonable memset.
>>>>>>
>>>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related
>>>>>> operations")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>> ---
>>>>>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c
>>>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>>>> index 510802be05..5a2cfe5a54 100644
>>>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>>>> int ret;
>>>>>> uint16_t i;
>>>>>> - memset(msg_data, 0, sizeof(uint16_t));
>>>>>> + memset(msg_data, 0, sizeof(msg_data));
>>>>>
>>>>> It looks a bit suspicious. May be it is better to do:
>>>>> memset(&msg_data, 0, sizeof(msg_data));
>>>> I think this is too hard to understand for &msg_data is **p.
>>>> maybe it confuse others when use memset to operate level-2 pointer.
>>>
>>> msg_data == &amsg_data
>>> since it is an array
>> Yes, you are right, the address of 'msg_data ' is the same
>> as the address of '&amsg_data'.
>>
>> But I still think this usage is not common.
>> like the current code in DPDK:
>> *****
>> static int
>> app_parse_flow_conf(const char *conf_str)
>> {
>> int ret;
>> uint32_t vals[5];
>> struct flow_conf *pconf;
>> uint64_t mask;
>>
>> memset(vals, 0, sizeof(vals));
>> ...
>> *****
>>
>> how about others's opinion? @Ferruh, @Thomas.
>>
>
> I would use array without address operator, so original patch lgtm.
thanks Ferruh.
>
> +1 to main purpose of the patch (to replace 'sizeof(uint16_t)').
> .
@@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
int ret;
uint16_t i;
- memset(msg_data, 0, sizeof(uint16_t));
+ memset(msg_data, 0, sizeof(msg_data));
ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
sizeof(msg_data), true, &reset_status,
sizeof(reset_status));