> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Thursday, February 25, 2016 4:36 PM
> To: Ananyev, Konstantin; dev at dpdk.org
> Cc: Zhang, Helin; nelio.laranjeiro at 6wind.com; adrien.mazarguil at
> 6wind.com; rahul.lakkireddy at chelsio.com
> Subject: Re: [PATCH v3 01/12] ethdev: add API to query packet type filling
> info
>
> Hi Konstantin,
>
> On 2/25/2016 11:46 PM, Ananyev, Konstantin wrote:
> > Hi Jainfeng,
> >
> >> Add a new API rte_eth_dev_get_ptype_info to query whether/what packet
> >> type can be filled by given pmd rx burst function.
> >>
> >> Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
> >> ---
> >> lib/librte_ether/rte_ethdev.c | 32 ++++++++++++++++++++++++++++++++
> >> lib/librte_ether/rte_ethdev.h | 23 +++++++++++++++++++++++
> >> 2 files changed, 55 insertions(+)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >> index 1257965..b52555b 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -1576,6 +1576,38 @@ rte_eth_dev_info_get(uint8_t port_id, struct
> >> rte_eth_dev_info *dev_info)
> >> dev_info->driver_name = dev->data->drv_name;
> >> }
> >>
> >> +int
> >> +rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptype_mask,
> >> + uint32_t **p_ptypes)
> >> +{
> >> + int i, j, ret;
> >> + struct rte_eth_dev *dev;
> >> + const uint32_t *all_ptypes;
> >> +
> >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >> + dev = &rte_eth_devices[port_id];
> >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_ptype_info_get, -ENOTSUP);
> >> + all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev);
> >> +
> >> + if (!all_ptypes)
> >> + return 0;
> >> +
> >> + for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
> >> + if (all_ptypes[i] & ptype_mask)
> >> + ret++;
> >> + if (ret == 0)
> >> + return 0;
> >> +
> >> + *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret);
> >> + if (*p_ptypes == NULL)
> >> + return -ENOMEM;
> >
> > I thought we all agreed to follow snprintf()-like logic and avoid any
> > memory allocations inside that function.
> > So why malloc() again?
> > Konstantin
> >
>
> Sorry for the setback. I still have concerns that snprintf()-like needs
> to be called twice by an application to get the ptype info. So I write
> this implementation for you to comment.
>
> So what's the reason why we should avoid memory allocations inside that
> function?
By the same reason none of other rte_ethdev get_info() style functions
(rte_eth_dev_info_get, rte_eth_rx_queue_info_get, rte_eth_xstats_get,
rte_eth_dev_rss_reta_query, etc) allocate space themselves.
It is a good practice to let user to decide himself how/where to allocate/free
a space for that date:
on a stack, inside a data segment (global variable), on heap (malloc),
at hugepages via rte_malloc, somewhere else.
BTW, if you had concerns about that approach, why didn't you provide any
arguments
when it was discussed/agreed?
Instead you came up a month later with same old approach that voids my and other
reviewers comments and even v2 of your own patch.
Do you have any good reason for that?
Konstantin
