Re: [dpdk-dev] [PATCH v12 02/12] librte_pcapng: add new library for writing pcapng files
From: Pattan, Reshma <hidden>
Date: 2021-10-15 09:36:06
-----Original Message----- From: dev <redacted> 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
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ buf = calloc(1, len); + if (!buf) + return -1;
How about returning -ENOMEM
quoted hunk ↗ jump to hunk
+ + 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?
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
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()?