Re: [dpdk-dev] [PATCH v10 06/12] pdump: support pcapng and filtering

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> 
> +++ b/lib/meson.build
> +        'bpf',
>          'bitratestats',

If alphabetical order,  should bpf come after bitratestats?

> +/*
> + * Note: version numbers intentionally start at 3
> + * in order to catch any application built with older out
> + * version of DPDK using incompatible client request format.
> + */
>  enum pdump_version {
> -	V1 = 1
> +	PDUMP_CLIENT_LEGACY = 3,
> +	PDUMP_CLIENT_PCAPNG = 4,
The version numbering was internal to library,  applications do not have control over  it, can't we start  enumeration from 1?

>  struct pdump_request {
> +	uint16_t flags;
Why is the flags type changed from unit32_t  unint16_t?

> 
> +		 * Similar behavior to rte_bpf_eth callback.
> +		 * if BPF program returns zero value for a given packet,
> +		 * then it will be ignored.
> +		 */
Looks like wrong callback name referred in the comment, should be corrected?

> +		if (cbs->filter && rcs[i] == 0) {
Why do we need to do this again if some packets already filtered.


> +	if (!(p->ver == PDUMP_CLIENT_LEGACY ||
> +	      p->ver == PDUMP_CLIENT_PCAPNG)) {
> +		PDUMP_LOG(ERR,
> +			  "incorrect client version %u\n", p->ver);
> +		return -EINVAL;
> +	}
This check is not useful here I guess, as we are setting the version in the library itself below.

> 
> +pdump_prepare_client_request(const char *device, uint16_t queue,
> +	req->queue = queue;
This assignment is done below as well, so here it is redundant I guess?

> -	} else {
> +		req->queue = queue;
>  	}
> 


> +	if (pdump_stats == NULL) {
> +		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +			PDUMP_LOG(ERR,
> +				  "pdump not initialized\n");
Might be god to say "pdump stats" not initialized  instead of just saying "pdump"?

> 
> +/**
> + * Retrieve the packet capture statistics for a queue.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param stats
> + *   A pointer to structure of type *rte_pdump_stats* to be filled in.
> + * @return
> + *   Zero if successful. -1 on error and rte_errno is set.
> + */
Missing below experimental warning in   the above comments .

> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> +notice