[PATCH 0/2] dumpcap,pdump handle cleanup signals

[PATCH 0/2] dumpcap,pdump handle cleanup signals

[Stephen Hemminger]

The dumpcap and pdump commands need to handle more signals.

Stephen Hemminger (2):
  app/dumpcap: handle SIGTERM and SIGHUP
  app/pdump: handle SIGTERM and SIGHUP

 app/dumpcap/main.c | 14 ++++++++++++--
 app/pdump/main.c   | 21 +++++++++++++++------
 2 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/2] app/dumpcap: handle SIGTERM and SIGHUP

[Stephen Hemminger]

If application is killed (SIGTERM), or the console session
ends (SIGHUP) or the write to the output file fails (SIGPIPE)
then do cleanup before exiting.

This also makes DPDK dumpcap behave more like Wireshark dumpcap.

Suggested-by: Isaac Boukris <iboukris@gmail.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index d57db0589a3f..76c747511444 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -939,6 +939,11 @@ int main(int argc, char **argv)
 {
 	struct rte_ring *r;
 	struct rte_mempool *mp;
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	dumpcap_out_t out;
 	char *p;
 
@@ -964,8 +969,13 @@ int main(int argc, char **argv)
 
 	compile_filters();
 
-	signal(SIGINT, signal_handler);
-	signal(SIGPIPE, SIG_IGN);
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	enable_primary_monitor();
 
-- 
2.43.0

[PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

Like dumpcap, the pdump process should cleanup if process
terminates due to being killed or hangup.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index a9205e130bb1..3592f8a865ad 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,11 +571,9 @@ disable_primary_monitor(void)
 }
 
 static void
-signal_handler(int sig_num)
+signal_handler(int sig_num __rte_unused)
 {
-	if (sig_num == SIGINT) {
-		quit_signal = 1;
-	}
+	quit_signal = 1;
 }
 
 static inline int
@@ -975,6 +973,11 @@ enable_primary_monitor(void)
 int
 main(int argc, char **argv)
 {
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	int diag;
 	int ret;
 	int i;
@@ -983,8 +986,14 @@ main(int argc, char **argv)
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 2];
 
-	/* catch ctrl-c so we can print on exit */
-	signal(SIGINT, signal_handler);
+	/* catch ctrl-c so we can cleanup on exit */
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	argp[0] = argv[0];
 	argp[1] = n_flag;
-- 
2.43.0

RE: [PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Pattan, Reshma]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> +	sigaction(SIGHUP, NULL, &origaction);
 
Why do we need this ?  Can't be this direct sigaction(SIGHUP, &action, NULL)  without a below if code? 
Can you please explain how this works. 

> +	if (origaction.sa_handler == SIG_DFL)
> +		sigaction(SIGHUP, &action, NULL);
> 


Other changes looks ok to me.

Regards,
Reshma

Re: [PATCH 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

On Tue, 27 Feb 2024 09:59:37 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > +	sigaction(SIGHUP, NULL, &origaction);  
>  
> Why do we need this ?  Can't be this direct sigaction(SIGHUP, &action, NULL)  without a below if code? 
> Can you please explain how this works. 
> 
> > +	if (origaction.sa_handler == SIG_DFL)
> > +		sigaction(SIGHUP, &action, NULL);
> >   
> 


If sighup is being ignored already (for example being run by nohup)
then the program should keep ignoring SIGHUP.

This is the method used in tshark and dumpcap utilities in wireshark
and it made sense.

[PATCH v2 0/2] Fix pdump and dumpcap leaks on SIGTERM

[Stephen Hemminger]

Fix bug where dumpcap and pdump programs would leave resources
if killed with SIGTERM (or if file gets full).

v2 - rebase and rewording

Stephen Hemminger (2):
  app/dumpcap: handle SIGTERM and SIGHUP
  app/pdump: handle SIGTERM and SIGHUP

 app/dumpcap/main.c | 14 ++++++++++++--
 app/pdump/main.c   | 21 +++++++++++++++------
 2 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] app/dumpcap: handle SIGTERM and SIGHUP

[Stephen Hemminger]

If the dumpcap is killed it does not cleanup which leaves
the ring buffer and the memory pool behind.
Until resource are exhausted devices will continue to fill the ring.

Fix by having the application handle SIGTERM, SIGHUP, and SIGPIPE.
These are the same signals handled in same way by the wireshark
dumpcap application.

Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index b25b95e2fc..ba91ca94d0 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -936,6 +936,11 @@ int main(int argc, char **argv)
 {
 	struct rte_ring *r;
 	struct rte_mempool *mp;
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	dumpcap_out_t out;
 	char *p;
 
@@ -961,8 +966,13 @@ int main(int argc, char **argv)
 
 	compile_filters();
 
-	signal(SIGINT, signal_handler);
-	signal(SIGPIPE, SIG_IGN);
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	enable_primary_monitor();
 
-- 
2.43.0

[PATCH v2 2/2] app/pdump: handle SIGTERM and SIGHUP

[Stephen Hemminger]

The pdump application will leak ring and memory pool if killed
by SIGTERM.  Like dumpcap, the pdump process should cleanup
if process terminates due to being killed or hangup.

Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3bdbb8dc78..fa85859703 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,11 +571,9 @@ disable_primary_monitor(void)
 }
 
 static void
-signal_handler(int sig_num)
+signal_handler(int sig_num __rte_unused)
 {
-	if (sig_num == SIGINT) {
-		quit_signal = 1;
-	}
+	quit_signal = 1;
 }
 
 static inline int
@@ -975,6 +973,11 @@ enable_primary_monitor(void)
 int
 main(int argc, char **argv)
 {
+	struct sigaction action = {
+		.sa_flags = SA_RESTART,
+		.sa_handler = signal_handler,
+	};
+	struct sigaction origaction;
 	int diag;
 	int ret;
 	int i;
@@ -983,8 +986,14 @@ main(int argc, char **argv)
 	char mp_flag[] = "--proc-type=secondary";
 	char *argp[argc + 2];
 
-	/* catch ctrl-c so we can print on exit */
-	signal(SIGINT, signal_handler);
+	/* catch ctrl-c so we can cleanup on exit */
+	sigemptyset(&action.sa_mask);
+	sigaction(SIGTERM, &action, NULL);
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGPIPE, &action, NULL);
+	sigaction(SIGHUP, NULL, &origaction);
+	if (origaction.sa_handler == SIG_DFL)
+		sigaction(SIGHUP, &action, NULL);
 
 	argp[0] = argv[0];
 	argp[1] = n_flag;
-- 
2.43.0