Re: [dpdk-dev] [PATCH v12 02/12] librte_pcapng: add new library for writing pcapng files

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

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> See draft RFC
>   https://www.ietf.org/id/draft-tuexen-opsawg-pcapng-03.html

The page is not found.  Might need to add new link I guess

> +enum pcapng_interface_options {
> +	PCAPNG_IFB_NAME	 = 2,
> +	PCAPNG_IFB_DESCRIPTION,

Can IFB(interface block) be replaced with IF(interface) only?  But that's ok, upto u.


> +	buf = calloc(1, len);
> +	if (!buf)
> +		return -1;

How about returning -ENOMEM

> +
> +	hdr = (struct pcapng_section_header *)buf;
> +	*hdr = (struct pcapng_section_header) {
> +		.block_type = PCAPNG_SECTION_BLOCK,
> +		.block_length = len,
> +		.byte_order_magic = PCAPNG_BYTE_ORDER_MAGIC,
> +		.major_version = PCAPNG_MAJOR_VERS,
> +		.minor_version = PCAPNG_MINOR_VERS,
> +		.section_length = UINT64_MAX,
> +	};
> +	hdr->block_length = len;

Why to assign block_len with len again? as it is already done few lines above.

> +	opt = pcapng_add_option(opt, PCAPNG_OPT_END, NULL, 0);

Some comments around this code, about adding end of options at the end of options list would be helpful.

> +
> +/* Write the PCAPNG section header at start of file */ static ssize_t

:s/section header/ interface header?

> +pcapng_interface_block(rte_pcapng_t *self, const char *if_name,
> +	if (mac_addr)
> +		len += pcapng_optlen(6);

How about using  RTE_ETHER_ADDR_LEN instead of 6

> +struct rte_mbuf * rte_pcapng_copy(uint16_t port_id, uint32_t queue,
<snip>
> +fail:
> +	rte_pktmbuf_free(mc);


Freeing mc , would that take care of freeing  up the additional byte prepended after mc creation?

> +	opt = pcapng_add_option(opt, PCAPNG_EPB_QUEUE,
> +				&queue, sizeof(queue));

Don't we need to add end of options to the end of option list, like did in Interface block and section header block?

> diff --git a/lib/pcapng/rte_pcapng.h b/lib/pcapng/rte_pcapng.h new file mode
> + *
> + * Packets to be captured are copied by rte_pcapng_mbuf()

Do you mean by rte_pcapng_copy()?