[dpdk-dev] Questions about API with no parameter check

[dpdk-dev] Questions about API with no parameter check

[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.
	Thanks.

Re: [dpdk-dev] Questions about API with no parameter check

[Thomas Monjalon]

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?

Re: [dpdk-dev] Questions about API with no parameter check

[Liang Ma]

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.

Re: [dpdk-dev] Questions about API with no parameter check

[Ananyev, Konstantin]

> 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

Re: [dpdk-dev] Questions about API with no parameter check

[Jerin Jacob]

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

Re: [dpdk-dev] Questions about API with no parameter check

[Ajit Khaparde]

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

Re: [dpdk-dev] Questions about API with no parameter check

[Hemant Agrawal]

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

Re: [dpdk-dev] Questions about API with no parameter check

[Ferruh Yigit]

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?

Re: [dpdk-dev] Questions about API with no parameter check

[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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] Questions about API with no parameter check

[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 ?

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
> 

Re: [dpdk-dev] Questions about API with no parameter check

[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


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

Re: [dpdk-dev] Questions about API with no parameter check

[Min Hu (Connor)]

在 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
>>>
>>
> 
> 
> 
> 
> 
> .
> 

Re: [dpdk-dev] Questions about API with no parameter check

[Tyler Retzlaff]

On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> >>+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?

why not assert them then, what is the purpose of returning an error to a
caller for a api contract violation like a `parameter shall not be NULL`

* assert.h/cassert can be compiled away for those pundits who don't want
  to see extra branches in their code

* when not compiled away it gives you an immediate stack trace or dump to operate
  on immediately identifying the problem instead of having to troll
  through hoaky inconsistently formatted logging.

* it catches callers who don't bother to check for error from return of
  the function (debug builds) instead of some arbitrary failure at some
  unrelated part of the code where the corrupted program state is relied
  upon.

we aren't running in kernel, we can crash.

Re: [dpdk-dev] Questions about API with no parameter check

[Dmitry Kozlyuk]

2021-04-29 09:16 (UTC-0700), Tyler Retzlaff:
> On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> > On 4/7/2021 4:25 PM, Hemant Agrawal wrote:  
> > >>+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?  
> 
> why not assert them then, what is the purpose of returning an error to a
> caller for a api contract violation like a `parameter shall not be NULL`
> 
> * assert.h/cassert can be compiled away for those pundits who don't want
>   to see extra branches in their code
>
> * when not compiled away it gives you an immediate stack trace or dump to operate
>   on immediately identifying the problem instead of having to troll
>   through hoaky inconsistently formatted logging.
> 
> * it catches callers who don't bother to check for error from return of
>   the function (debug builds) instead of some arbitrary failure at some
>   unrelated part of the code where the corrupted program state is relied
>   upon.
> 
> we aren't running in kernel, we can crash.

As library developers we can't assume stability requirements at call site.
There may be temporary files to clean up, for example,
or other threads in the middle of their work.

As an application developer I'd hate to get a crash inside a library and
having to debug it. Usually installed are release versions with assertions
compiled away.

Log formatting is the only point I agree with, but it's another huge topic.

Re: [dpdk-dev] Questions about API with no parameter check

[Tyler Retzlaff]

On Thu, Apr 29, 2021 at 09:49:24PM +0300, Dmitry Kozlyuk wrote:
> 2021-04-29 09:16 (UTC-0700), Tyler Retzlaff:
> > On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> > > On 4/7/2021 4:25 PM, Hemant Agrawal wrote:  
> > > >>+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?  
> > 
> > why not assert them then, what is the purpose of returning an error to a
> > caller for a api contract violation like a `parameter shall not be NULL`
> > 
> > * assert.h/cassert can be compiled away for those pundits who don't want
> >   to see extra branches in their code
> >
> > * when not compiled away it gives you an immediate stack trace or dump to operate
> >   on immediately identifying the problem instead of having to troll
> >   through hoaky inconsistently formatted logging.
> > 
> > * it catches callers who don't bother to check for error from return of
> >   the function (debug builds) instead of some arbitrary failure at some
> >   unrelated part of the code where the corrupted program state is relied
> >   upon.
> > 
> > we aren't running in kernel, we can crash.
> 
> As library developers we can't assume stability requirements at call site.
> There may be temporary files to clean up, for example,
> or other threads in the middle of their work.

if a callers state is so incoherent that it is passing NULL to functions
that contractually expect non-NULL it is already way past the point of
no return. continuing to run only accomplishes destroying the state that
might be used to diagnose the originating flaw in program logic.

if you return an error instead of fail fast at best you'll crash soon but
more often then not you'll keep running and produce incorrect results or worst
keep running security compromised.

about the only argument that can be made for having this silly error
pattern that is valid is when many-party code is running inside the same
process and you don't want someone elses bad code taking your process
down. a problem that i am accutely aware of in allowing 3rd party code run
in kernel space. (but this is mostly? mitigated by multi-process mode).

> As an application developer I'd hate to get a crash inside a library and
> having to debug it. Usually installed are release versions with assertions
> compiled away.
> 

so it wouldn't crash at all at least not at the point of failure. the only
difference is i guess you wouldn't get a log message with what is being done
now.

could we turn this around and have it tunable by policy instead of
opting everyone in to this behavior maybe?  i'm just making some ideas up on
the fly but couldn't we just have something that is compile time policy?

#ifdef EAL_FAILURE_POLICY_RETURN
#define EAL_FAILURE(condition, error) \
if ((condition)) { \
    return (error); \
}
#else
#define EAL_FAILURE(condition, error) \
    assert(! (condition), (error));
#endif

also, i'll point out that lately there have been a lot of patches
accepted that call functions and don't evaluate their return value and
the reason is those functions really should never have been "failable".
so we'll just see more of that as we stack on often compile time or
immediate runtime failure returns. of course the compatibility of the
code calling these functions is only as good as the implicit dependency
on the implementation... until it changes and the application
misbehaves.

i'll also throw another gripe in here that there are a lot of
"deallocation" functions in dpdk that according to their api can fail
again because of this kind of "oh i'll fail because i got a bad
parameter design".

deallocation should never fail ever and i shouldn't need to write logic
around a deallocation to handle failures. imagine if free failed?

p = malloc(...);
if (p == NULL)
     return -1;

... do work with p ...

rv = free(p);
if (rv != 0) ... what the hell? yet this pattern exists in a bunch of
places. it's insane. (i'll quietly ignore the design error that free
does accept NULL and is a noop standardized *facepalm*).

anyway, i guess i've ranted enough. there are some users who would
prefer not to have this but i admit there are an overwhelming number of
people who seem to want it.

Re: [dpdk-dev] Questions about API with no parameter check

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Friday, April 30, 2021 2:16 AM
> 
> On Thu, Apr 29, 2021 at 09:49:24PM +0300, Dmitry Kozlyuk wrote:
> > 2021-04-29 09:16 (UTC-0700), Tyler Retzlaff:
> > > On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> > > > On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> > > > >>+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?
> > >
> > > why not assert them then, what is the purpose of returning an error
> to a
> > > caller for a api contract violation like a `parameter shall not be
> NULL`
> > >
> > > * assert.h/cassert can be compiled away for those pundits who don't
> want
> > >   to see extra branches in their code
> > >
> > > * when not compiled away it gives you an immediate stack trace or
> dump to operate
> > >   on immediately identifying the problem instead of having to troll
> > >   through hoaky inconsistently formatted logging.
> > >
> > > * it catches callers who don't bother to check for error from
> return of
> > >   the function (debug builds) instead of some arbitrary failure at
> some
> > >   unrelated part of the code where the corrupted program state is
> relied
> > >   upon.
> > >
> > > we aren't running in kernel, we can crash.
> >
> > As library developers we can't assume stability requirements at call
> site.
> > There may be temporary files to clean up, for example,
> > or other threads in the middle of their work.
> 
> if a callers state is so incoherent that it is passing NULL to
> functions
> that contractually expect non-NULL it is already way past the point of
> no return. continuing to run only accomplishes destroying the state
> that
> might be used to diagnose the originating flaw in program logic.
> 
> if you return an error instead of fail fast at best you'll crash soon
> but
> more often then not you'll keep running and produce incorrect results
> or worst
> keep running security compromised.
> 
> about the only argument that can be made for having this silly error
> pattern that is valid is when many-party code is running inside the
> same
> process and you don't want someone elses bad code taking your process
> down. a problem that i am accutely aware of in allowing 3rd party code
> run
> in kernel space. (but this is mostly? mitigated by multi-process mode).
> 
> > As an application developer I'd hate to get a crash inside a library
> and
> > having to debug it. Usually installed are release versions with
> assertions
> > compiled away.
> >
> 
> so it wouldn't crash at all at least not at the point of failure. the
> only
> difference is i guess you wouldn't get a log message with what is being
> done
> now.
> 
> could we turn this around and have it tunable by policy instead of
> opting everyone in to this behavior maybe?  i'm just making some ideas
> up on
> the fly but couldn't we just have something that is compile time
> policy?
> 
> #ifdef EAL_FAILURE_POLICY_RETURN
> #define EAL_FAILURE(condition, error) \
> if ((condition)) { \
>     return (error); \
> }
> #else
> #define EAL_FAILURE(condition, error) \
>     assert(! (condition), (error));
> #endif
> 

I agree with the overall idea - it's better to fail immediately when a violation is detected. And more asserts are better than fewer.

However, I don't see the need for a completely new macro. For testing contract violations and similar, we already have RTE_VERIFY() and RTE_ASSERT(), where the latter can be controlled by RTE_ENABLE_ASSERT at compile time.

> also, i'll point out that lately there have been a lot of patches
> accepted that call functions and don't evaluate their return value and
> the reason is those functions really should never have been "failable".
> so we'll just see more of that as we stack on often compile time or
> immediate runtime failure returns. of course the compatibility of the
> code calling these functions is only as good as the implicit dependency
> on the implementation... until it changes and the application
> misbehaves.
> 
> i'll also throw another gripe in here that there are a lot of
> "deallocation" functions in dpdk that according to their api can fail
> again because of this kind of "oh i'll fail because i got a bad
> parameter design".
> 
> deallocation should never fail ever and i shouldn't need to write logic
> around a deallocation to handle failures. imagine if free failed?
> 
> p = malloc(...);
> if (p == NULL)
>      return -1;
> 
> ... do work with p ...
> 
> rv = free(p);
> if (rv != 0) ... what the hell? yet this pattern exists in a bunch of
> places. it's insane. (i'll quietly ignore the design error that free
> does accept NULL and is a noop standardized *facepalm*).
> 
> anyway, i guess i've ranted enough. there are some users who would
> prefer not to have this but i admit there are an overwhelming number of
> people who seem to want it.

Good rant. More thoughts should be put into API design. We certainly don't need failure return values for functions that shouldn't be able to fail in the first place.

Application errors are caught earlier in the development process, when libraries fail hard (e.g. rte_panic()) on errors instead of trying to handle them gracefully!

Re: [dpdk-dev] Questions about API with no parameter check

[Ananyev, Konstantin]

> 
> 2021-04-29 09:16 (UTC-0700), Tyler Retzlaff:
> > On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> > > On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> > > >>+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?
> >
> > why not assert them then, what is the purpose of returning an error to a
> > caller for a api contract violation like a `parameter shall not be NULL`
> >
> > * assert.h/cassert can be compiled away for those pundits who don't want
> >   to see extra branches in their code
> >
> > * when not compiled away it gives you an immediate stack trace or dump to operate
> >   on immediately identifying the problem instead of having to troll
> >   through hoaky inconsistently formatted logging.
> >
> > * it catches callers who don't bother to check for error from return of
> >   the function (debug builds) instead of some arbitrary failure at some
> >   unrelated part of the code where the corrupted program state is relied
> >   upon.
> >
> > we aren't running in kernel, we can crash.
> 
> As library developers we can't assume stability requirements at call site.
> There may be temporary files to clean up, for example,
> or other threads in the middle of their work.
> 
> As an application developer I'd hate to get a crash inside a library and
> having to debug it. Usually installed are release versions with assertions
> compiled away.

I agree with Dmitry summary above.
Asserting inside the library calls is bad programming practice,
please keep it away from the project. 

> 
> Log formatting is the only point I agree with, but it's another huge topic.

Re: [dpdk-dev] Questions about API with no parameter check

[Tyler Retzlaff]

On Tue, May 04, 2021 at 09:36:24AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > 
> > 2021-04-29 09:16 (UTC-0700), Tyler Retzlaff:
> > > On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote:
> > > > On 4/7/2021 4:25 PM, Hemant Agrawal wrote:
> > > > >>+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?
> > >
> > > why not assert them then, what is the purpose of returning an error to a
> > > caller for a api contract violation like a `parameter shall not be NULL`
> > >
> > > * assert.h/cassert can be compiled away for those pundits who don't want
> > >   to see extra branches in their code
> > >
> > > * when not compiled away it gives you an immediate stack trace or dump to operate
> > >   on immediately identifying the problem instead of having to troll
> > >   through hoaky inconsistently formatted logging.
> > >
> > > * it catches callers who don't bother to check for error from return of
> > >   the function (debug builds) instead of some arbitrary failure at some
> > >   unrelated part of the code where the corrupted program state is relied
> > >   upon.
> > >
> > > we aren't running in kernel, we can crash.
> > 
> > As library developers we can't assume stability requirements at call site.
> > There may be temporary files to clean up, for example,
> > or other threads in the middle of their work.
> > 
> > As an application developer I'd hate to get a crash inside a library and
> > having to debug it. Usually installed are release versions with assertions
> > compiled away.
> 
> I agree with Dmitry summary above.
> Asserting inside the library calls is bad programming practice,
> please keep it away from the project. 

i'm not advocating for asserts i'm advocating for users to have a
choice instead of being opted in to this change unconditionally.

asserts are an option that may be policy controlled as previously mentioned
either in this thread or another. so if you don't like them you can disable
them as a function of the policy. for a basic assert that means building
release instead of debug but a more sophisticated policy mechanism could be
employed if desired.

what you can't turn off is introduction of superfluous errors being
returned due to programming mistakes in the application which should be
handled yet have no sensible way to be handled. it just clutters the
calling code with unnecessary error handling, makes the errors returned
ambiguious and often indistinguishable from real errors.

by this logic we should modify rte_free to be

int rte_free(void * p) { if (p == NULL) return EINVAL; mem_free(p, true); }

which is about as useful as one can imagine.

this proposal has been pushed through too quickly without proper debate,
and the patch that introduces the superfluous errors breaks abi. tech
board should get involved before it goes further.

i'm not asking for asserts, i'm asking not to be opted in to an equally
harmful error handling pattern that makes application logic more error
prone and more complex negatively impacting quality.

thanks