Re: [PATCH 05/12] net/hns3: fix an unreasonable memset

["Min Hu (Connor)" <humin29@huawei.com>]

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));
>>>
>>> .
> 
> .