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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 15 Oct 2021 09:36:00 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> > -----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.

Ok, but someone looking at this code should really look at the standard
to see what the data format is.

> > +
> > +/* Write the PCAPNG section header at start of file */ static ssize_t  
> 
> :s/section header/ interface header?

Good catch, copy/paste of comment.

> 
> > +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

Fixing now, also merging pcapng_interface_block since only called one place.


> 
> > +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?

Mbuf are allocation unit, so the whole buffer goes.

> 
> > +	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?

It turns out that the reference (wireshark) does not. So did not do
that to save space on the output file.

> 
> > 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()?

Good catch, function got renamed and comment not updated.