> -----Original Message-----
> From: dev <dev-boun...@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
