[v3,2/5] dumpcap: allow multiple invocations
Checks
Commit Message
If dumpcap is run twice with each instance pointing a different
interface, it would fail because of overlap in ring a pool names.
Fix by putting process id in the name.
Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")
Reported-by: Isaac Boukris <iboukris@gmail.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/dumpcap/main.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
Comments
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 8 November 2023 19.36
>
> If dumpcap is run twice with each instance pointing a different
> interface, it would fail because of overlap in ring a pool names.
> Fix by putting process id in the name.
>
> Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")
> Reported-by: Isaac Boukris <iboukris@gmail.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Minor detail: getpid() returns int, so prefer %d over %u.
[...]
> rte_exit(EXIT_FAILURE,
> "Packet dump enable on %u:%s failed %s\n",
> intf->port, intf->name,
> - rte_strerror(-ret));
> + rte_strerror(rte_errno));
This bugfix (the line above, not the patch itself) supports Tyler's proposal to standardize on returning -1 with rte_errno set on failure, instead of some functions returning -errno. Our dual convention for function return values will cause many bugs like this.
With %u or %d,
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
On Thu, 9 Nov 2023 08:50:10 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
>
> Minor detail: getpid() returns int, so prefer %d over %u.
Let me check, per man page. getpid() returns pid_t.
The typedef chain leads to:
pid_t -> __pid_t -> __PID_T_TYPE -> __S32_TYPE -> int32 -> int
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 9 November 2023 16.40
>
> On Thu, 9 Nov 2023 08:50:10 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> >
> > Minor detail: getpid() returns int, so prefer %d over %u.
>
> Let me check, per man page. getpid() returns pid_t.
> The typedef chain leads to:
> pid_t -> __pid_t -> __PID_T_TYPE -> __S32_TYPE -> int32 -> int
Thank you for confirming. So %d is preferred over %u for getpid(). :-)
On Thu, 9 Nov 2023 08:50:10 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> > rte_exit(EXIT_FAILURE,
> > "Packet dump enable on %u:%s failed %s\n",
> > intf->port, intf->name,
> > - rte_strerror(-ret));
> > + rte_strerror(rte_errno));
>
> This bugfix (the line above, not the patch itself) supports Tyler's proposal to standardize on returning -1 with rte_errno set on failure, instead of some functions returning -errno. Our dual convention for function return values will cause many bugs like this.
The error case here is when rte_pdump_enable_bpf() fails.
This is return from pdump_enable in pdump library.
The library does follow the rte_errno convention correctly.
But the error message wasn't reporting correctly which would lead to confusing error in case where
multiple invocations failed.
It is not possible to do multiple captures on same interface. And not worth modifying the
library (would require multiple copies and ref counts) to handle this case.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 9 November 2023 18.16
>
> On Thu, 9 Nov 2023 08:50:10 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > > rte_exit(EXIT_FAILURE,
> > > "Packet dump enable on %u:%s failed %s\n",
> > > intf->port, intf->name,
> > > - rte_strerror(-ret));
> > > + rte_strerror(rte_errno));
> >
> > This bugfix (the line above, not the patch itself) supports Tyler's
> proposal to standardize on returning -1 with rte_errno set on failure,
> instead of some functions returning -errno. Our dual convention for
> function return values will cause many bugs like this.
>
> The error case here is when rte_pdump_enable_bpf() fails.
> This is return from pdump_enable in pdump library.
> The library does follow the rte_errno convention correctly.
I'm sorry about being unclear in my comment about rte_errno conventions; it was not targeted at this library.
My comment was meant as general support for Tyler's suggestion, using this as an example of a bug that would not have been there if the return convention was always -1 with rte_errno.
With the dual return convention, it's amazing that you caught this bug.
> But the error message wasn't reporting correctly which would lead to
> confusing error in case where
> multiple invocations failed.
>
> It is not possible to do multiple captures on same interface. And not
> worth modifying the
> library (would require multiple copies and ref counts) to handle this
> case.
@@ -44,7 +44,6 @@
#include <pcap/pcap.h>
#include <pcap/bpf.h>
-#define RING_NAME "capture-ring"
#define MONITOR_INTERVAL (500 * 1000)
#define MBUF_POOL_CACHE_SIZE 32
#define BURST_SIZE 32
@@ -647,6 +646,7 @@ static void dpdk_init(void)
static struct rte_ring *create_ring(void)
{
struct rte_ring *ring;
+ char ring_name[RTE_RING_NAMESIZE];
size_t size, log2;
/* Find next power of 2 >= size. */
@@ -660,28 +660,28 @@ static struct rte_ring *create_ring(void)
ring_size = size;
}
- ring = rte_ring_lookup(RING_NAME);
- if (ring == NULL) {
- ring = rte_ring_create(RING_NAME, ring_size,
- rte_socket_id(), 0);
- if (ring == NULL)
- rte_exit(EXIT_FAILURE, "Could not create ring :%s\n",
- rte_strerror(rte_errno));
- }
+ /* Want one ring per invocation of program */
+ snprintf(ring_name, sizeof(ring_name),
+ "dumpcap-%u", getpid());
+
+ ring = rte_ring_create(ring_name, ring_size,
+ rte_socket_id(), 0);
+ if (ring == NULL)
+ rte_exit(EXIT_FAILURE, "Could not create ring :%s\n",
+ rte_strerror(rte_errno));
+
return ring;
}
static struct rte_mempool *create_mempool(void)
{
const struct interface *intf;
- static const char pool_name[] = "capture_mbufs";
+ char pool_name[RTE_MEMPOOL_NAMESIZE];
size_t num_mbufs = 2 * ring_size;
struct rte_mempool *mp;
uint32_t data_size = 128;
- mp = rte_mempool_lookup(pool_name);
- if (mp)
- return mp;
+ snprintf(pool_name, sizeof(pool_name), "capture_%u", getpid());
/* Common pool so size mbuf for biggest snap length */
TAILQ_FOREACH(intf, &interfaces, next) {
@@ -826,7 +826,7 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
rte_exit(EXIT_FAILURE,
"Packet dump enable on %u:%s failed %s\n",
intf->port, intf->name,
- rte_strerror(-ret));
+ rte_strerror(rte_errno));
}
if (intf->opts.promisc_mode) {