[dpdk-dev] [PATCH 0/2] ethdev: use strlcpy

[dpdk-dev] [PATCH 0/2] ethdev: use strlcpy

[Stephen Hemminger]

A couple of places in ethdev are clearer if strlcpy is used.

Stephen Hemminger (2):
  ethdev: replace snprintf with strlcpy
  ethdev: use strlcpy instead of snprintf on initialization

 lib/librte_ethdev/rte_ethdev.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] ethdev: replace snprintf with strlcpy

[Stephen Hemminger]

The set_port_owner was copying a string between structures of the
same type, therefore the name could never be truncated (unless source
string was not null terminated).  Use strlcpy which does it better.

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

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 85c1794968dd..95889ed206db 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -585,7 +585,6 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 {
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
-	int sret;
 
 	if (port_id >= RTE_MAX_ETHPORTS || !is_allocated(ethdev)) {
 		RTE_ETHDEV_LOG(ERR, "Port id %"PRIu16" is not allocated\n",
@@ -609,11 +608,8 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		return -EPERM;
 	}
 
-	sret = snprintf(port_owner->name, RTE_ETH_MAX_OWNER_NAME_LEN, "%s",
-			new_owner->name);
-	if (sret < 0 || sret >= RTE_ETH_MAX_OWNER_NAME_LEN)
-		RTE_ETHDEV_LOG(ERR, "Port %u owner name was truncated\n",
-			port_id);
+	/* can not truncate (same structure) */
+	strlcpy(port_owner->name, new_owner->name, RTE_ETH_MAX_OWNER_NAME_LEN);
 
 	port_owner->id = new_owner->id;
 
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Stephen Hemminger]

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);
 	eth_dev->data->port_id = port_id;
 	eth_dev->data->mtu = ETHER_MTU;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Rami Rosen]

Reviewed-by: Rami Rosen <ramirose@gmail.com>

Re: [dpdk-dev] [PATCH 1/2] ethdev: replace snprintf with strlcpy

[Andrew Rybchenko]

On 3/1/19 1:47 AM, Stephen Hemminger wrote:
> The set_port_owner was copying a string between structures of the
> same type, therefore the name could never be truncated (unless source
> string was not null terminated).  Use strlcpy which does it better.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Andrew Rybchenko]

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

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Stephen Hemminger]

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

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Andrew Rybchenko]

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.

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH 2/2] ethdev: use strlcpy instead of snprintf on initialization

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH 0/2] ethdev: use strlcpy

[Ferruh Yigit]

On 2/28/2019 10:47 PM, Stephen Hemminger wrote:
> A couple of places in ethdev are clearer if strlcpy is used.
> 
> Stephen Hemminger (2):
>   ethdev: replace snprintf with strlcpy
>   ethdev: use strlcpy instead of snprintf on initialization

Series applied to dpdk-next-net/master, thanks.

(2/2 updated as suggested while merging.)