On Fri, 15 Oct 2021 09:36:00 +0000 "Pattan, Reshma" <reshma.pat...@intel.com> wrote:
> > -----Original Message----- > > From: dev <dev-boun...@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.
