Re: [dpdk-dev] [PATCH v2] ethdev: enhance the API for getting burst mode information

[Thomas Monjalon <thomas@monjalon.net>]

06/11/2019 02:21, Wang, Haiyue:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 04/11/2019 11:39, Haiyue Wang:
> > >  /**
> > >   * Ethernet device RX/TX queue packet burst mode information structure.
> > >   * Used to retrieve information about packet burst mode setting.
> > >   */
> > >  struct rte_eth_burst_mode {
> > > -	uint64_t options;
> > > +	uint64_t flags; /**< The ORed values of RTE_ETH_BURST_FLAG_xxx */
> > > +
> > > +#define RTE_ETH_BURST_MODE_INFO_SIZE 1024 /**< Maximum size for information */
> > > +	char info[RTE_ETH_BURST_MODE_INFO_SIZE]; /**< burst mode information */
> > >  };
> > 
> > I think the API can be simpler by passing the flags as function parameter.
> > 
> > In my understanding the burst mode name is fixed per Rx/Tx function,
> > so it can be a constant string referenced with a simple char*.
> > 
> > This is the current API:
> > 
> > int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> >     struct rte_eth_burst_mode *mode);
> > 
> > I wonder what do you think of such API? (just a proposal for comments):
> > 
> > char *rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id, uint64_t flags);
> > 
> > Or is there some cases where you want to build the string with snprintf?
> > (I cannot think about a case, given it should mapped to a C-function)
> > 
> 
> 1. 'a constant string' is hard for PMD expanding if it wants to make the string
>    dynamic according to the setting, like: http://patchwork.dpdk.org/patch/62352/
>    (although based on bit options design).

Yes, constant string is less flexible in the PMD implementation.

> 2. And for dynamic string, if it is *return type*, then the PMD needs to
>    handle the memory allocation, and the application frees it. And 'uint64_t flags'
>    is output parameter, so it should be like 'uint64_t *flags', but this needs the
>    application to declare it or not, and needs PMDs to check whether it is passed
>    or not, then set it.
> 
>    So for making things easy, the 'struct rte_eth_burst_mode' may be nice, then the
>    application just declares one line : 'struct rte_eth_burst_mode mode', then all
>    things are filled by PMD in one place.

I agree it is a lot simpler to use a struct.