Hi Andrew, Thomas > -----Original Message----- > From: Thomas Monjalon <tho...@monjalon.net> > Sent: Thursday, October 24, 2019 6:35 PM > To: Andrew Rybchenko <arybche...@solarflare.com> > Cc: dev@dpdk.org; Ori Kam <or...@mellanox.com>; Ferruh Yigit > <ferruh.yi...@intel.com>; jingjing...@intel.com; > step...@networkplumber.org > Subject: Re: [dpdk-dev] [PATCH v5 02/15] ethdev: add support for hairpin queue > > 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&data=02%7C01%7Corika%40mellanox.com%7C022cd953964f4a20d50508d7508a259f%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637066426584029629&sdata=G8ZxEWFFsv1kLWc6L7sKT8O6CSiBcj5ZuwQqmK0Q6nY%3D&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?
