[rdma-core] srp_daemon: Install signal handler for ibsrpdm

Commit Message

Honggang LI Dec. 14, 2017, 11:02 a.m. UTC

From: Honggang Li <[e-mail]>

[root@localhost ~]# /usr/sbin/ibsrpdm -c -d /dev/infiniband/umad0
id_ext=0002c90300317810,ioc_guid=0002c90300317810,dgid=fe800000000000000002c90300317811,pkey=ffff,service_id=0002c90300317810

[root@localhost ~]# echo $?
130

fd3005f0cd34 moves the signal handler setup from ibsrpdm path. So,
default signal handler will be used when it receives signal SIGINT.
ibsrpdm will exit with non-zero exit code as default signal handler
killed it. ibsrpdm should return with exit code zero, if no error
emerged.

main->ibsrpdm->free_res->pthread_kill(res->async_ev_thread, SIGINT)

Beside the wrong exit code, there is an extra blank line at the end of
output of ibsrpdm.

Fixes: fd3005f0cd34 ("srp_daemon: Move the setup of the wakeup_pipe after openlog")
Signed-off-by: Honggang Li <[e-mail]>
---
 srp_daemon/srp_daemon.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Bart Van Assche Dec. 14, 2017, 2:59 p.m. UTC | #1

On Thu, 2017-12-14 at 19:02 +0800, Honggang LI wrote:
> fd3005f0cd34 moves the signal handler setup from ibsrpdm path. So,

> default signal handler will be used when it receives signal SIGINT.

> ibsrpdm will exit with non-zero exit code as default signal handler

> killed it.


Can you explain why you think that ibsrpdm needs a signal handler?

> ibsrpdm should return with exit code zero, if no error emerged.


That's correct, but if it was interrupted by a signal handler an error
code should be returned.

Bart.

Honggang LI Dec. 15, 2017, 1:36 a.m. UTC | #2

On Thu, Dec 14, 2017 at 02:59:34PM +0000, Bart Van Assche wrote:
> On Thu, 2017-12-14 at 19:02 +0800, Honggang LI wrote:
> > fd3005f0cd34 moves the signal handler setup from ibsrpdm path. So,
> > default signal handler will be used when it receives signal SIGINT.
> > ibsrpdm will exit with non-zero exit code as default signal handler
> > killed it.
> 
> Can you explain why you think that ibsrpdm needs a signal handler?

main
  ibsrpdm
    alloc_res
      pthread_create(&res->res.async_ev_thread [1]
    ....
    free_res
      if (res->async_ev_thread) { pthread_kill(res->async_ev_thread, SIGINT); [2]


The ibsrpdm program create a thread in [1], and send a SIGINT in [2].
The default behavior of SIGINT is to terminate the process.

According to the man page of pthread_kill:
 Signal dispositions are process-wide: if a signal handler is installed,
 the handler will be invoked in the thread _thread_, but if the disposition 
 of the signal is "stop", "continue", or "terminate", this action will 
 affect the whole process.

If we did not install a signal handler for ibsrpdm, ibsrpdm will be
terminated in [2], when it free the resource. ibsrpdm expected to return
to the main program after it free the resource. However, it failed as it
killed by default signal handler.

> 
> > ibsrpdm should return with exit code zero, if no error emerged.
> 
> That's correct, but if it was interrupted by a signal handler an error
> code should be returned.
> 
> Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index cec36db2..0d00d712 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -1996,7 +1996,6 @@  static void cleanup_wakeup_fd(void)
 
 static int setup_wakeup_fd(void)
 {
-	struct sigaction sa = {};
 	int ret;
 
 	ret = pipe2(wakeup_pipe, O_NONBLOCK | O_CLOEXEC);
@@ -2005,11 +2004,6 @@  static int setup_wakeup_fd(void)
 		return -1;
 	}
 
-	sigemptyset(&sa.sa_mask);
-	sa.sa_handler = signal_handler;
-	sigaction(SIGINT, &sa, NULL);
-	sigaction(SIGTERM, &sa, NULL);
-	sigaction(SRP_CATAS_ERR, &sa, NULL);
 	return 0;
 }
 
@@ -2100,6 +2094,7 @@  int main(int argc, char *argv[])
 	int			lockfd = -1;
 	int			received_signal = 0;
 	bool                    systemd;
+	struct sigaction        sa = {};
 
 #ifndef __CHECKER__
 	/*
@@ -2117,6 +2112,12 @@  int main(int argc, char *argv[])
 	BUILD_ASSERT(sizeof(struct srp_dm_iou_info) == 132);
 	BUILD_ASSERT(sizeof(struct srp_dm_ioc_prof) == 128);
 
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = signal_handler;
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
+	sigaction(SRP_CATAS_ERR, &sa, NULL);
+
 	if (strcmp(argv[0] + max_t(int, 0, strlen(argv[0]) - strlen("ibsrpdm")),
 		   "ibsrpdm") == 0) {
 		ret = ibsrpdm(argc, argv);