DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] Questions about API with no parameter check
@ 2021-04-07 11:28 Min Hu (Connor)
  2021-04-07 11:40 ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Min Hu (Connor) @ 2021-04-07 11:28 UTC (permalink / raw)
  To: dev, Ferruh Yigit, Andrew Rybchenko, Thomas Monjalon

Hi, all,
	Many APIs in DPDK does not check if the pointer parameter is
NULL or not. For example, in 'rte_ethdev.c':
int
rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
		       uint16_t nb_rx_desc, unsigned int socket_id,
		       const struct rte_eth_rxconf *rx_conf,
		       struct rte_mempool *mp)

int
rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)

int
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)

int
rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)

As these APIs could be used by any APPs, if the APP give NULL as
the pointer parameter, segmetation default will occur.

So, my question is, should we add check in the API? like that,
int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
{
	if (stats == NULL)
		return -EINVAL;
	...
}

Or, that is redundant, the parameter correctness should be guaranteed by
the APP?

What's your opinion? Hope for your reply.
	Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 11:28 [dpdk-dev] Questions about API with no parameter check Min Hu (Connor)
@ 2021-04-07 11:40 ` Thomas Monjalon
  2021-04-07 11:48   ` Liang Ma
  2021-04-07 11:53   ` Ananyev, Konstantin
  0 siblings, 2 replies; 12+ messages in thread
From: Thomas Monjalon @ 2021-04-07 11:40 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko, Min Hu (Connor)
  Cc: dev, olivier.matz, david.marchand, jerinj, ajit.khaparde,
	hemant.agrawal, bruce.richardson

07/04/2021 13:28, Min Hu (Connor):
> Hi, all,
> 	Many APIs in DPDK does not check if the pointer parameter is
> NULL or not. For example, in 'rte_ethdev.c':
> int
> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> 		       uint16_t nb_rx_desc, unsigned int socket_id,
> 		       const struct rte_eth_rxconf *rx_conf,
> 		       struct rte_mempool *mp)
> 
> int
> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> 
> int
> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> 
> int
> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> 
> As these APIs could be used by any APPs, if the APP give NULL as
> the pointer parameter, segmetation default will occur.
> 
> So, my question is, should we add check in the API? like that,
> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> {
> 	if (stats == NULL)
> 		return -EINVAL;
> 	...
> }
> 
> Or, that is redundant, the parameter correctness should be guaranteed by
> the APP?
> 
> What's your opinion? Hope for your reply.

I remember it has been discussed in the past (many years ago),
and the opinion was to not clutter the code for something that
is a basic fault from the app.

I don't have a strong opinion.
What is your opinion? Others?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 11:40 ` Thomas Monjalon
@ 2021-04-07 11:48   ` Liang Ma
  2021-04-07 11:53   ` Ananyev, Konstantin
  1 sibling, 0 replies; 12+ messages in thread
From: Liang Ma @ 2021-04-07 11:48 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Ferruh Yigit, Andrew Rybchenko, Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, ajit.khaparde,
	hemant.agrawal, bruce.richardson

On Wed, Apr 07, 2021 at 01:40:32PM +0200, Thomas Monjalon wrote:
> 07/04/2021 13:28, Min Hu (Connor):
> > Hi, all,
> > 	Many APIs in DPDK does not check if the pointer parameter is
> > NULL or not. For example, in 'rte_ethdev.c':
> > int
> > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > 		       uint16_t nb_rx_desc, unsigned int socket_id,
> > 		       const struct rte_eth_rxconf *rx_conf,
> > 		       struct rte_mempool *mp)
> > 
> > int
> > rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> > 
> > int
> > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > 
> > int
> > rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> > 
> > As these APIs could be used by any APPs, if the APP give NULL as
> > the pointer parameter, segmetation default will occur.
> > 
> > So, my question is, should we add check in the API? like that,
> > int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > {
> > 	if (stats == NULL)
> > 		return -EINVAL;
> > 	...
> > }
> > 
> > Or, that is redundant, the parameter correctness should be guaranteed by
> > the APP?
> > 
> > What's your opinion? Hope for your reply.
> 
> I remember it has been discussed in the past (many years ago),
> and the opinion was to not clutter the code for something that
> is a basic fault from the app.
> 
> I don't have a strong opinion.
> What is your opinion? Others?
Sure. Application control the lifetime cycle of the memory region. API
impl can check it but everyone pay penalty(branch) as default. Let App
developer make the decision is the best choice IMO.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 11:40 ` Thomas Monjalon
  2021-04-07 11:48   ` Liang Ma
@ 2021-04-07 11:53   ` Ananyev, Konstantin
  2021-04-07 13:19     ` Jerin Jacob
  1 sibling, 1 reply; 12+ messages in thread
From: Ananyev, Konstantin @ 2021-04-07 11:53 UTC (permalink / raw)
  To: Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko, Min Hu (Connor)
  Cc: dev, olivier.matz, david.marchand, jerinj, ajit.khaparde,
	hemant.agrawal, Richardson, Bruce



> 07/04/2021 13:28, Min Hu (Connor):
> > Hi, all,
> > 	Many APIs in DPDK does not check if the pointer parameter is
> > NULL or not. For example, in 'rte_ethdev.c':
> > int
> > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > 		       uint16_t nb_rx_desc, unsigned int socket_id,
> > 		       const struct rte_eth_rxconf *rx_conf,
> > 		       struct rte_mempool *mp)
> >
> > int
> > rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> >
> > int
> > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> >
> > int
> > rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> >
> > As these APIs could be used by any APPs, if the APP give NULL as
> > the pointer parameter, segmetation default will occur.
> >
> > So, my question is, should we add check in the API? like that,
> > int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > {
> > 	if (stats == NULL)
> > 		return -EINVAL;
> > 	...
> > }
> >
> > Or, that is redundant, the parameter correctness should be guaranteed by
> > the APP?
> >
> > What's your opinion? Hope for your reply.
> 
> I remember it has been discussed in the past (many years ago),
> and the opinion was to not clutter the code for something that
> is a basic fault from the app.
> 
> I don't have a strong opinion.
> What is your opinion? Others?

As I can see these are control path functions.
So some extra formal parameters check wouldn't hurt.
+1 from me to add them.
Konstantin



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 11:53   ` Ananyev, Konstantin
@ 2021-04-07 13:19     ` Jerin Jacob
  2021-04-07 14:40       ` Ajit Khaparde
  0 siblings, 1 reply; 12+ messages in thread
From: Jerin Jacob @ 2021-04-07 13:19 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko, Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, ajit.khaparde,
	hemant.agrawal, Richardson, Bruce

On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
>
>
> > 07/04/2021 13:28, Min Hu (Connor):
> > > Hi, all,
> > >     Many APIs in DPDK does not check if the pointer parameter is
> > > NULL or not. For example, in 'rte_ethdev.c':
> > > int
> > > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > >                    uint16_t nb_rx_desc, unsigned int socket_id,
> > >                    const struct rte_eth_rxconf *rx_conf,
> > >                    struct rte_mempool *mp)
> > >
> > > int
> > > rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> > >
> > > int
> > > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > >
> > > int
> > > rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> > >
> > > As these APIs could be used by any APPs, if the APP give NULL as
> > > the pointer parameter, segmetation default will occur.
> > >
> > > So, my question is, should we add check in the API? like that,
> > > int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > > {
> > >     if (stats == NULL)
> > >             return -EINVAL;
> > >     ...
> > > }
> > >
> > > Or, that is redundant, the parameter correctness should be guaranteed by
> > > the APP?
> > >
> > > What's your opinion? Hope for your reply.
> >
> > I remember it has been discussed in the past (many years ago),
> > and the opinion was to not clutter the code for something that
> > is a basic fault from the app.
> >
> > I don't have a strong opinion.
> > What is your opinion? Others?
>
> As I can see these are control path functions.
> So some extra formal parameters check wouldn't hurt.
> +1 from me to add them.

+1 to add more sanity checks in control path APIs


> Konstantin
>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 13:19     ` Jerin Jacob
@ 2021-04-07 14:40       ` Ajit Khaparde
  2021-04-07 15:25         ` Hemant Agrawal
  0 siblings, 1 reply; 12+ messages in thread
From: Ajit Khaparde @ 2021-04-07 14:40 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Ananyev, Konstantin, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko, Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, hemant.agrawal,
	Richardson, Bruce

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
> >
> >
> >
> > > 07/04/2021 13:28, Min Hu (Connor):
> > > > Hi, all,
> > > >     Many APIs in DPDK does not check if the pointer parameter is
> > > > NULL or not. For example, in 'rte_ethdev.c':
> > > > int
> > > > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > > >                    uint16_t nb_rx_desc, unsigned int socket_id,
> > > >                    const struct rte_eth_rxconf *rx_conf,
> > > >                    struct rte_mempool *mp)
> > > >
> > > > int
> > > > rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> > > >
> > > > int
> > > > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > > >
> > > > int
> > > > rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> > > >
> > > > As these APIs could be used by any APPs, if the APP give NULL as
> > > > the pointer parameter, segmetation default will occur.
> > > >
> > > > So, my question is, should we add check in the API? like that,
> > > > int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> > > > {
> > > >     if (stats == NULL)
> > > >             return -EINVAL;
> > > >     ...
> > > > }
> > > >
> > > > Or, that is redundant, the parameter correctness should be guaranteed by
> > > > the APP?
> > > >
> > > > What's your opinion? Hope for your reply.
> > >
> > > I remember it has been discussed in the past (many years ago),
> > > and the opinion was to not clutter the code for something that
> > > is a basic fault from the app.
> > >
> > > I don't have a strong opinion.
> > > What is your opinion? Others?
> >
> > As I can see these are control path functions.
> > So some extra formal parameters check wouldn't hurt.
> > +1 from me to add them.
>
> +1 to add more sanity checks in control path APIs
+1
But are we going to check all parameters?

>
>
> > Konstantin
> >
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 14:40       ` Ajit Khaparde
@ 2021-04-07 15:25         ` Hemant Agrawal
  2021-04-07 16:10           ` Ferruh Yigit
  0 siblings, 1 reply; 12+ messages in thread
From: Hemant Agrawal @ 2021-04-07 15:25 UTC (permalink / raw)
  To: Ajit Khaparde, Jerin Jacob
  Cc: Ananyev, Konstantin, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko, Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, hemant.agrawal,
	Richardson, Bruce


On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
>> <konstantin.ananyev@intel.com> wrote:
>>>
>>>
>>>> 07/04/2021 13:28, Min Hu (Connor):
>>>>> Hi, all,
>>>>>      Many APIs in DPDK does not check if the pointer parameter is
>>>>> NULL or not. For example, in 'rte_ethdev.c':
>>>>> int
>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>>>                     uint16_t nb_rx_desc, unsigned int socket_id,
>>>>>                     const struct rte_eth_rxconf *rx_conf,
>>>>>                     struct rte_mempool *mp)
>>>>>
>>>>> int
>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>>>>
>>>>> int
>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>
>>>>> int
>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>>>>
>>>>> As these APIs could be used by any APPs, if the APP give NULL as
>>>>> the pointer parameter, segmetation default will occur.
>>>>>
>>>>> So, my question is, should we add check in the API? like that,
>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>> {
>>>>>      if (stats == NULL)
>>>>>              return -EINVAL;
>>>>>      ...
>>>>> }
>>>>>
>>>>> Or, that is redundant, the parameter correctness should be guaranteed by
>>>>> the APP?
>>>>>
>>>>> What's your opinion? Hope for your reply.
>>>> I remember it has been discussed in the past (many years ago),
>>>> and the opinion was to not clutter the code for something that
>>>> is a basic fault from the app.
>>>>
>>>> I don't have a strong opinion.
>>>> What is your opinion? Others?
>>> As I can see these are control path functions.
>>> So some extra formal parameters check wouldn't hurt.
>>> +1 from me to add them.
>> +1 to add more sanity checks in control path APIs
> +1
> But are we going to check all parameters?

+1

It may be better to limit the number of checks.

>>
>>> Konstantin
>>>
>>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 15:25         ` Hemant Agrawal
@ 2021-04-07 16:10           ` Ferruh Yigit
  2021-04-07 16:26             ` Burakov, Anatoly
  0 siblings, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2021-04-07 16:10 UTC (permalink / raw)
  To: hemant.agrawal, Ajit Khaparde, Jerin Jacob
  Cc: Ananyev, Konstantin, Thomas Monjalon, Andrew Rybchenko,
	Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, Richardson, Bruce

On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> 
> On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
>> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
>>> <konstantin.ananyev@intel.com> wrote:
>>>>
>>>>
>>>>> 07/04/2021 13:28, Min Hu (Connor):
>>>>>> Hi, all,
>>>>>>      Many APIs in DPDK does not check if the pointer parameter is
>>>>>> NULL or not. For example, in 'rte_ethdev.c':
>>>>>> int
>>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>>>>                     uint16_t nb_rx_desc, unsigned int socket_id,
>>>>>>                     const struct rte_eth_rxconf *rx_conf,
>>>>>>                     struct rte_mempool *mp)
>>>>>>
>>>>>> int
>>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>>>>>
>>>>>> int
>>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>>
>>>>>> int
>>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>>>>>
>>>>>> As these APIs could be used by any APPs, if the APP give NULL as
>>>>>> the pointer parameter, segmetation default will occur.
>>>>>>
>>>>>> So, my question is, should we add check in the API? like that,
>>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>> {
>>>>>>      if (stats == NULL)
>>>>>>              return -EINVAL;
>>>>>>      ...
>>>>>> }
>>>>>>
>>>>>> Or, that is redundant, the parameter correctness should be guaranteed by
>>>>>> the APP?
>>>>>>
>>>>>> What's your opinion? Hope for your reply.
>>>>> I remember it has been discussed in the past (many years ago),
>>>>> and the opinion was to not clutter the code for something that
>>>>> is a basic fault from the app.
>>>>>
>>>>> I don't have a strong opinion.
>>>>> What is your opinion? Others?
>>>> As I can see these are control path functions.
>>>> So some extra formal parameters check wouldn't hurt.
>>>> +1 from me to add them.
>>> +1 to add more sanity checks in control path APIs
>> +1
>> But are we going to check all parameters?
> 
> +1
> 
> It may be better to limit the number of checks.
> 

+1 to verify input for APIs.

Why not do all, what is the downside of checking all input for control path APIs?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 16:10           ` Ferruh Yigit
@ 2021-04-07 16:26             ` Burakov, Anatoly
  2021-04-08  1:06               ` Min Hu (Connor)
  0 siblings, 1 reply; 12+ messages in thread
From: Burakov, Anatoly @ 2021-04-07 16:26 UTC (permalink / raw)
  To: Ferruh Yigit, hemant.agrawal, Ajit Khaparde, Jerin Jacob
  Cc: Ananyev, Konstantin, Thomas Monjalon, Andrew Rybchenko,
	Min Hu (Connor),
	dev, olivier.matz, david.marchand, jerinj, Richardson, Bruce

On 07-Apr-21 5:10 PM, Ferruh Yigit wrote:
> On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
>>
>> On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
>>> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> 
>>> wrote:
>>>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com> wrote:
>>>>>
>>>>>
>>>>>> 07/04/2021 13:28, Min Hu (Connor):
>>>>>>> Hi, all,
>>>>>>>      Many APIs in DPDK does not check if the pointer parameter is
>>>>>>> NULL or not. For example, in 'rte_ethdev.c':
>>>>>>> int
>>>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>>>>>                     uint16_t nb_rx_desc, unsigned int socket_id,
>>>>>>>                     const struct rte_eth_rxconf *rx_conf,
>>>>>>>                     struct rte_mempool *mp)
>>>>>>>
>>>>>>> int
>>>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>>>>>>
>>>>>>> int
>>>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>>>
>>>>>>> int
>>>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info 
>>>>>>> *dev_info)
>>>>>>>
>>>>>>> As these APIs could be used by any APPs, if the APP give NULL as
>>>>>>> the pointer parameter, segmetation default will occur.
>>>>>>>
>>>>>>> So, my question is, should we add check in the API? like that,
>>>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>>> {
>>>>>>>      if (stats == NULL)
>>>>>>>              return -EINVAL;
>>>>>>>      ...
>>>>>>> }
>>>>>>>
>>>>>>> Or, that is redundant, the parameter correctness should be 
>>>>>>> guaranteed by
>>>>>>> the APP?
>>>>>>>
>>>>>>> What's your opinion? Hope for your reply.
>>>>>> I remember it has been discussed in the past (many years ago),
>>>>>> and the opinion was to not clutter the code for something that
>>>>>> is a basic fault from the app.
>>>>>>
>>>>>> I don't have a strong opinion.
>>>>>> What is your opinion? Others?
>>>>> As I can see these are control path functions.
>>>>> So some extra formal parameters check wouldn't hurt.
>>>>> +1 from me to add them.
>>>> +1 to add more sanity checks in control path APIs
>>> +1
>>> But are we going to check all parameters?
>>
>> +1
>>
>> It may be better to limit the number of checks.
>>
> 
> +1 to verify input for APIs.
> 
> Why not do all, what is the downside of checking all input for control 
> path APIs?
> 

+1

Don't have anything useful to add that hasn't already been said, but 
seems like a nice +1-train going on here, so i thought i'd hop on board :D

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-07 16:26             ` Burakov, Anatoly
@ 2021-04-08  1:06               ` Min Hu (Connor)
  2021-04-08  8:22                 ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Min Hu (Connor) @ 2021-04-08  1:06 UTC (permalink / raw)
  To: Burakov, Anatoly, Ferruh Yigit, hemant.agrawal, Ajit Khaparde,
	Jerin Jacob
  Cc: Ananyev, Konstantin, Thomas Monjalon, Andrew Rybchenko, dev,
	olivier.matz, david.marchand, jerinj, Richardson, Bruce

Thanks all,
Well, Most people who replied support input verification for APIs.
As the APIs are in control path APIs, so checking all input is OK.

This is a large project because there are so many APIs and libs in DPDK.
I will send a set of patches to fix that.

Thomas, Ferruh, and others, any opinions ?

Thanks.


在 2021/4/8 0:26, Burakov, Anatoly 写道:
> On 07-Apr-21 5:10 PM, Ferruh Yigit wrote:
>> On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
>>>
>>> On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
>>>> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> 
>>>> wrote:
>>>>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
>>>>> <konstantin.ananyev@intel.com> wrote:
>>>>>>
>>>>>>
>>>>>>> 07/04/2021 13:28, Min Hu (Connor):
>>>>>>>> Hi, all,
>>>>>>>>      Many APIs in DPDK does not check if the pointer parameter is
>>>>>>>> NULL or not. For example, in 'rte_ethdev.c':
>>>>>>>> int
>>>>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>>>>>>                     uint16_t nb_rx_desc, unsigned int socket_id,
>>>>>>>>                     const struct rte_eth_rxconf *rx_conf,
>>>>>>>>                     struct rte_mempool *mp)
>>>>>>>>
>>>>>>>> int
>>>>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>>>>>>>
>>>>>>>> int
>>>>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>>>>
>>>>>>>> int
>>>>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info 
>>>>>>>> *dev_info)
>>>>>>>>
>>>>>>>> As these APIs could be used by any APPs, if the APP give NULL as
>>>>>>>> the pointer parameter, segmetation default will occur.
>>>>>>>>
>>>>>>>> So, my question is, should we add check in the API? like that,
>>>>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats 
>>>>>>>> *stats)
>>>>>>>> {
>>>>>>>>      if (stats == NULL)
>>>>>>>>              return -EINVAL;
>>>>>>>>      ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> Or, that is redundant, the parameter correctness should be 
>>>>>>>> guaranteed by
>>>>>>>> the APP?
>>>>>>>>
>>>>>>>> What's your opinion? Hope for your reply.
>>>>>>> I remember it has been discussed in the past (many years ago),
>>>>>>> and the opinion was to not clutter the code for something that
>>>>>>> is a basic fault from the app.
>>>>>>>
>>>>>>> I don't have a strong opinion.
>>>>>>> What is your opinion? Others?
>>>>>> As I can see these are control path functions.
>>>>>> So some extra formal parameters check wouldn't hurt.
>>>>>> +1 from me to add them.
>>>>> +1 to add more sanity checks in control path APIs
>>>> +1
>>>> But are we going to check all parameters?
>>>
>>> +1
>>>
>>> It may be better to limit the number of checks.
>>>
>>
>> +1 to verify input for APIs.
>>
>> Why not do all, what is the downside of checking all input for control 
>> path APIs?
>>
> 
> +1
> 
> Don't have anything useful to add that hasn't already been said, but 
> seems like a nice +1-train going on here, so i thought i'd hop on board :D
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-08  1:06               ` Min Hu (Connor)
@ 2021-04-08  8:22                 ` Thomas Monjalon
  2021-04-08  9:00                   ` Min Hu (Connor)
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Monjalon @ 2021-04-08  8:22 UTC (permalink / raw)
  To: Burakov, Anatoly, Ferruh Yigit, hemant.agrawal, Ajit Khaparde,
	Jerin Jacob, Min Hu (Connor)
  Cc: Ananyev, Konstantin, Andrew Rybchenko, dev, olivier.matz,
	david.marchand, jerinj, Richardson, Bruce

08/04/2021 03:06, Min Hu (Connor):
> Thanks all,
> Well, Most people who replied support input verification for APIs.
> As the APIs are in control path APIs, so checking all input is OK.
> 
> This is a large project because there are so many APIs and libs in DPDK.
> I will send a set of patches to fix that.
> 
> Thomas, Ferruh, and others, any opinions ?

Let's start with ethdev and we'll see if it looks a good addition,
and if it is well accepted in the community.
Thanks


> 在 2021/4/8 0:26, Burakov, Anatoly 写道:
> > On 07-Apr-21 5:10 PM, Ferruh Yigit wrote:
> >> On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> >>>
> >>> On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
> >>>> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com> 
> >>>> wrote:
> >>>>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
> >>>>> <konstantin.ananyev@intel.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>> 07/04/2021 13:28, Min Hu (Connor):
> >>>>>>>> Hi, all,
> >>>>>>>>      Many APIs in DPDK does not check if the pointer parameter is
> >>>>>>>> NULL or not. For example, in 'rte_ethdev.c':
> >>>>>>>> int
> >>>>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> >>>>>>>>                     uint16_t nb_rx_desc, unsigned int socket_id,
> >>>>>>>>                     const struct rte_eth_rxconf *rx_conf,
> >>>>>>>>                     struct rte_mempool *mp)
> >>>>>>>>
> >>>>>>>> int
> >>>>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> >>>>>>>>
> >>>>>>>> int
> >>>>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> >>>>>>>>
> >>>>>>>> int
> >>>>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info 
> >>>>>>>> *dev_info)
> >>>>>>>>
> >>>>>>>> As these APIs could be used by any APPs, if the APP give NULL as
> >>>>>>>> the pointer parameter, segmetation default will occur.
> >>>>>>>>
> >>>>>>>> So, my question is, should we add check in the API? like that,
> >>>>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats 
> >>>>>>>> *stats)
> >>>>>>>> {
> >>>>>>>>      if (stats == NULL)
> >>>>>>>>              return -EINVAL;
> >>>>>>>>      ...
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> Or, that is redundant, the parameter correctness should be 
> >>>>>>>> guaranteed by
> >>>>>>>> the APP?
> >>>>>>>>
> >>>>>>>> What's your opinion? Hope for your reply.
> >>>>>>> I remember it has been discussed in the past (many years ago),
> >>>>>>> and the opinion was to not clutter the code for something that
> >>>>>>> is a basic fault from the app.
> >>>>>>>
> >>>>>>> I don't have a strong opinion.
> >>>>>>> What is your opinion? Others?
> >>>>>> As I can see these are control path functions.
> >>>>>> So some extra formal parameters check wouldn't hurt.
> >>>>>> +1 from me to add them.
> >>>>> +1 to add more sanity checks in control path APIs
> >>>> +1
> >>>> But are we going to check all parameters?
> >>>
> >>> +1
> >>>
> >>> It may be better to limit the number of checks.
> >>>
> >>
> >> +1 to verify input for APIs.
> >>
> >> Why not do all, what is the downside of checking all input for control 
> >> path APIs?
> >>
> > 
> > +1
> > 
> > Don't have anything useful to add that hasn't already been said, but 
> > seems like a nice +1-train going on here, so i thought i'd hop on board :D
> > 
> 






^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] Questions about API with no parameter check
  2021-04-08  8:22                 ` Thomas Monjalon
@ 2021-04-08  9:00                   ` Min Hu (Connor)
  0 siblings, 0 replies; 12+ messages in thread
From: Min Hu (Connor) @ 2021-04-08  9:00 UTC (permalink / raw)
  To: Thomas Monjalon, Burakov, Anatoly, Ferruh Yigit, hemant.agrawal,
	Ajit Khaparde, Jerin Jacob
  Cc: Ananyev, Konstantin, Andrew Rybchenko, dev, olivier.matz,
	david.marchand, jerinj, Richardson, Bruce



在 2021/4/8 16:22, Thomas Monjalon 写道:
> 08/04/2021 03:06, Min Hu (Connor):
>> Thanks all,
>> Well, Most people who replied support input verification for APIs.
>> As the APIs are in control path APIs, so checking all input is OK.
>>
>> This is a large project because there are so many APIs and libs in DPDK.
>> I will send a set of patches to fix that.
>>
>> Thomas, Ferruh, and others, any opinions ?
> 
> Let's start with ethdev and we'll see if it looks a good addition,
> and if it is well accepted in the community.
> Thanks
> 
Got it, thanks Thomas. I will send one patch to fix ethdev.
> 
>> 在 2021/4/8 0:26, Burakov, Anatoly 写道:
>>> On 07-Apr-21 5:10 PM, Ferruh Yigit wrote:
>>>> On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
>>>>>
>>>>> On 4/7/2021 8:10 PM, Ajit Khaparde wrote:
>>>>>> On Wed, Apr 7, 2021 at 6:20 AM Jerin Jacob <jerinjacobk@gmail.com>
>>>>>> wrote:
>>>>>>> On Wed, Apr 7, 2021 at 5:23 PM Ananyev, Konstantin
>>>>>>> <konstantin.ananyev@intel.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> 07/04/2021 13:28, Min Hu (Connor):
>>>>>>>>>> Hi, all,
>>>>>>>>>>       Many APIs in DPDK does not check if the pointer parameter is
>>>>>>>>>> NULL or not. For example, in 'rte_ethdev.c':
>>>>>>>>>> int
>>>>>>>>>> rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>>>>>>>>                      uint16_t nb_rx_desc, unsigned int socket_id,
>>>>>>>>>>                      const struct rte_eth_rxconf *rx_conf,
>>>>>>>>>>                      struct rte_mempool *mp)
>>>>>>>>>>
>>>>>>>>>> int
>>>>>>>>>> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>>>>>>>>>
>>>>>>>>>> int
>>>>>>>>>> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>>>>>>>>>
>>>>>>>>>> int
>>>>>>>>>> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info
>>>>>>>>>> *dev_info)
>>>>>>>>>>
>>>>>>>>>> As these APIs could be used by any APPs, if the APP give NULL as
>>>>>>>>>> the pointer parameter, segmetation default will occur.
>>>>>>>>>>
>>>>>>>>>> So, my question is, should we add check in the API? like that,
>>>>>>>>>> int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats
>>>>>>>>>> *stats)
>>>>>>>>>> {
>>>>>>>>>>       if (stats == NULL)
>>>>>>>>>>               return -EINVAL;
>>>>>>>>>>       ...
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Or, that is redundant, the parameter correctness should be
>>>>>>>>>> guaranteed by
>>>>>>>>>> the APP?
>>>>>>>>>>
>>>>>>>>>> What's your opinion? Hope for your reply.
>>>>>>>>> I remember it has been discussed in the past (many years ago),
>>>>>>>>> and the opinion was to not clutter the code for something that
>>>>>>>>> is a basic fault from the app.
>>>>>>>>>
>>>>>>>>> I don't have a strong opinion.
>>>>>>>>> What is your opinion? Others?
>>>>>>>> As I can see these are control path functions.
>>>>>>>> So some extra formal parameters check wouldn't hurt.
>>>>>>>> +1 from me to add them.
>>>>>>> +1 to add more sanity checks in control path APIs
>>>>>> +1
>>>>>> But are we going to check all parameters?
>>>>>
>>>>> +1
>>>>>
>>>>> It may be better to limit the number of checks.
>>>>>
>>>>
>>>> +1 to verify input for APIs.
>>>>
>>>> Why not do all, what is the downside of checking all input for control
>>>> path APIs?
>>>>
>>>
>>> +1
>>>
>>> Don't have anything useful to add that hasn't already been said, but
>>> seems like a nice +1-train going on here, so i thought i'd hop on board :D
>>>
>>
> 
> 
> 
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-04-08  9:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 11:28 [dpdk-dev] Questions about API with no parameter check Min Hu (Connor)
2021-04-07 11:40 ` Thomas Monjalon
2021-04-07 11:48   ` Liang Ma
2021-04-07 11:53   ` Ananyev, Konstantin
2021-04-07 13:19     ` Jerin Jacob
2021-04-07 14:40       ` Ajit Khaparde
2021-04-07 15:25         ` Hemant Agrawal
2021-04-07 16:10           ` Ferruh Yigit
2021-04-07 16:26             ` Burakov, Anatoly
2021-04-08  1:06               ` Min Hu (Connor)
2021-04-08  8:22                 ` Thomas Monjalon
2021-04-08  9:00                   ` Min Hu (Connor)

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git