Re: [dpdk-dev] [PATCH v5 02/15] ethdev: add support for hairpin queue

[Thomas Monjalon <thomas@monjalon.net>]

25/10/2019 21:01, Ori Kam:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 24/10/2019 17:30, Andrew Rybchenko:
> > > On 10/24/19 6:17 PM, Thomas Monjalon wrote:
> > > > 24/10/2019 16:47, Andrew Rybchenko:
> > > >> On 10/24/19 11:29 AM, Ori Kam wrote:
> > > >>> Hi Andrew,
> > > >>>
> > > >>> When writing the new function I thought about using bool, but
> > > >>> I decided against it for the following reasons:
> > > >>> 1. There is no use of bool any where in the code, and there is not special
> > reason to add it now.
> > > >> rte_ethdev.c includes stdbool.h and uses bool
> > > >>
> > > >>> 2. Other functions of this kind already returns int. for example
> > (rte_eth_dev_is_valid_port / rte_eth_is_valid_owner_id)
> > > > I agree with Ori here for 2 reasons:
> > > > 1. It is better to be consistent in the API
> > > > 2. I remember having some issues with some drivers when introducing
> > stdbool in the API.
> > > >
> > > > I think it may be nice to convert all such API to bool in one patch,
> > > > and check if there are some remaining issues with bool usage in drivers or
> > with PPC.
> > > > But I suggest to do such API change in DPDK 20.11.
> > >
> > > OK, no problem. Does it prevent to avoid comparison == 1? Just to
> > > avoid changes in these lines in the future.
> > 
> > Yes probably better to avoid explicit comparison, but prefer boolean operator
> > (!).
> > 
> > 
> 
> Thomas I understand your comments but from Andrew comment on my V2-01 patch:
> "
> >>> +	ret = (*dev->dev_ops->rx_hairpin_queue_setup)(dev, rx_queue_id,
> >>> +						      nb_rx_desc, conf);
> >>> +	if (!ret)
> >> Please, compare with 0
> >>
> > Will do, but again just for my knowledge why?
> 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html%23function-calls&amp;data=02%7C01%7Corika%40mellanox.com%7C022cd953964f4a20d50508d7508a259f%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637066426584029629&amp;sdata=G8ZxEWFFsv1kLWc6L7sKT8O6CSiBcj5ZuwQqmK0Q6nY%3D&amp;reserved=0
> 
> "
> I don't see any relevant info in the link, but maybe I'm missing something .
> What are the rules? 
> Thomas also keep in mind that in most cases the condition that is tested is the positive, meaning  it will look something like this:
> if (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id)) {     
>         rte_errno = EINVAL;                               
>         return NULL;                                      
> }                                       
> 
> What do you think?

I think for normal functions with error codes,
we should compare explictly with a value.
But for boolean-type functions like "is_hairpin_queue",
we should have implicit (natural) comparison. So yes, this is correct:
	if (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id))