diff mbox series

[2/2] ethdev: use strlcpy instead of snprintf on initialization

Message ID 20190228224754.26511-3-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: use strlcpy |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Feb. 28, 2019, 10:47 p.m. UTC 
  Don't need to use snprintf for simple name copy.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ethdev/rte_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rami Rosen March 1, 2019, 6:54 a.m. UTC | #1 
Reviewed-by: Rami Rosen <ramirose@gmail.com>
Andrew Rybchenko March 1, 2019, 7:48 a.m. UTC | #2 
On 3/1/19 1:47 AM, Stephen Hemminger wrote:
> Don't need to use snprintf for simple name copy.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 95889ed206db..8bd54dcf58c1 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -459,7 +459,7 @@ rte_eth_dev_allocate(const char *name)
>   	}
>   
>   	eth_dev = eth_dev_get(port_id);
> -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> +	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);

Why is sizeof() substituted with RTE_ETH_NAME_MAX_LEN?
I thought that sizeof() is the first choice in such cases since it is a 
bit more
safer vs possible changes in the code.

BTW, wouldn't it be more friendly to check name length on entry and
reject if it is too long? (and same for rte_eth_dev_create())
I agree that strlcpy() should be used anyway.

>   	eth_dev->data->port_id = port_id;
>   	eth_dev->data->mtu = ETHER_MTU;
>
Stephen Hemminger March 1, 2019, 6:42 p.m. UTC | #3 
On Fri, 1 Mar 2019 10:48:58 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 3/1/19 1:47 AM, Stephen Hemminger wrote:
> > Don't need to use snprintf for simple name copy.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 95889ed206db..8bd54dcf58c1 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -459,7 +459,7 @@ rte_eth_dev_allocate(const char *name)
> >   	}
> >   
> >   	eth_dev = eth_dev_get(port_id);
> > -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> > +	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);  
> 
> Why is sizeof() substituted with RTE_ETH_NAME_MAX_LEN?

Same thing, I just wanted to make the length obvious to the reader.

> I thought that sizeof() is the first choice in such cases since it is a 
> bit more
> safer vs possible changes in the code.
> 
> BTW, wouldn't it be more friendly to check name length on entry and
> reject if it is too long? (and same for rte_eth_dev_create())

It is impossible for name to long since since both structures are the same.

> I agree that strlcpy() should be used anyway.
> 
> >   	eth_dev->data->port_id = port_id;
> >   	eth_dev->data->mtu = ETHER_MTU;
> >     
>
Andrew Rybchenko March 4, 2019, 9:11 a.m. UTC | #4 
On 3/1/19 9:42 PM, Stephen Hemminger wrote:
> On Fri, 1 Mar 2019 10:48:58 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 3/1/19 1:47 AM, Stephen Hemminger wrote:
>>> Don't need to use snprintf for simple name copy.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>>    lib/librte_ethdev/rte_ethdev.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 95889ed206db..8bd54dcf58c1 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -459,7 +459,7 @@ rte_eth_dev_allocate(const char *name)
>>>    	}
>>>    
>>>    	eth_dev = eth_dev_get(port_id);
>>> -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>>> +	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);
>> Why is sizeof() substituted with RTE_ETH_NAME_MAX_LEN?
> Same thing, I just wanted to make the length obvious to the reader.
>
>> I thought that sizeof() is the first choice in such cases since it is a
>> bit more
>> safer vs possible changes in the code.
>>
>> BTW, wouldn't it be more friendly to check name length on entry and
>> reject if it is too long? (and same for rte_eth_dev_create())
> It is impossible for name to long since since both structures are the same.

Which structures? name is an input parameter of the function.
Stephen Hemminger March 4, 2019, 7:12 p.m. UTC | #5 
On Mon, 4 Mar 2019 12:11:20 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 3/1/19 9:42 PM, Stephen Hemminger wrote:
> > On Fri, 1 Mar 2019 10:48:58 +0300
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >  
> >> On 3/1/19 1:47 AM, Stephen Hemminger wrote:  
> >>> Don't need to use snprintf for simple name copy.
> >>>
> >>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>> ---
> >>>    lib/librte_ethdev/rte_ethdev.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>> index 95889ed206db..8bd54dcf58c1 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -459,7 +459,7 @@ rte_eth_dev_allocate(const char *name)
> >>>    	}
> >>>    
> >>>    	eth_dev = eth_dev_get(port_id);
> >>> -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> >>> +	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);  
> >> Why is sizeof() substituted with RTE_ETH_NAME_MAX_LEN?  
> > Same thing, I just wanted to make the length obvious to the reader.
> >  
> >> I thought that sizeof() is the first choice in such cases since it is a
> >> bit more
> >> safer vs possible changes in the code.
> >>
> >> BTW, wouldn't it be more friendly to check name length on entry and
> >> reject if it is too long? (and same for rte_eth_dev_create())  
> > It is impossible for name to long since since both structures are the same.  
> 
> Which structures? name is an input parameter of the function.
> 

Sorry my confusion, that was in patch 1.
Ferruh Yigit March 6, 2019, 3:55 p.m. UTC | #6 
On 3/4/2019 7:12 PM, Stephen Hemminger wrote:
> On Mon, 4 Mar 2019 12:11:20 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
>> On 3/1/19 9:42 PM, Stephen Hemminger wrote:
>>> On Fri, 1 Mar 2019 10:48:58 +0300
>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>  
>>>> On 3/1/19 1:47 AM, Stephen Hemminger wrote:  
>>>>> Don't need to use snprintf for simple name copy.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>>> ---
>>>>>    lib/librte_ethdev/rte_ethdev.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>>> index 95889ed206db..8bd54dcf58c1 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> @@ -459,7 +459,7 @@ rte_eth_dev_allocate(const char *name)
>>>>>    	}
>>>>>    
>>>>>    	eth_dev = eth_dev_get(port_id);
>>>>> -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>>>>> +	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);  
>>>> Why is sizeof() substituted with RTE_ETH_NAME_MAX_LEN?  
>>> Same thing, I just wanted to make the length obvious to the reader.
>>>  
>>>> I thought that sizeof() is the first choice in such cases since it is a
>>>> bit more
>>>> safer vs possible changes in the code.
>>>>
>>>> BTW, wouldn't it be more friendly to check name length on entry and
>>>> reject if it is too long? (and same for rte_eth_dev_create())  
>>> It is impossible for name to long since since both structures are the same.  
>>
>> Which structures? name is an input parameter of the function.
>>
> 
> Sorry my confusion, that was in patch 1.

Only concern is keep using "sizeof(eth_dev->data->name)" or
RTE_ETH_NAME_MAX_LEN, both are correct but I agree with Andrew about using sizeof.


Stephen,

Would you be OK if I update it to sizeof() while merging, so I can merge them?

Thanks,
ferruh
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 95889ed206db..8bd54dcf58c1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -459,7 +459,7 @@  rte_eth_dev_allocate(const char *name)
 	}
 
 	eth_dev = eth_dev_get(port_id);
-	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
+	strlcpy(eth_dev->data->name, name, RTE_ETH_NAME_MAX_LEN);
 	eth_dev->data->port_id = port_id;
 	eth_dev->data->mtu = ETHER_MTU;