Hi aVu,
"[Vu] Use "else {" instead (without if) in order to cover all error cases?"
Look like function "KillProc()", I think we don't need handle printout in
the case with the error "ESRCH" (The process or process group specified in
pid cannot be found) ?
Regards
Canh
-----Original Message-----
From: Vu Minh Nguyen <[email protected]>
Sent: Wednesday, May 9, 2018 4:33 PM
To: 'Canh Van Truong' <[email protected]>;
[email protected]; [email protected]
Cc: [email protected]
Subject: RE: [PATCH 1/1] ntf: remove sending SIGTERM and SIGABRT when
killing osafntfimcnd [#2851]
Hi Canh,
Ack with a minor comment, started with [Vu].
Regards, Vu
> -----Original Message-----
> From: Canh Van Truong <[email protected]>
> Sent: Wednesday, May 9, 2018 3:30 PM
> To: [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; Canh Van Truong
> <[email protected]>
> Subject: [PATCH 1/1] ntf: remove sending SIGTERM and SIGABRT when killing
> osafntfimcnd [#2851]
>
> The handler of SIGTERM is just _Exit, so using SIGTERM/SIGABRT is not
> needed.
> The patch changes to send SIGKILL when terminating osafntfimcnd process.
> ---
> src/ntf/ntfd/ntfs_imcnutil.c | 136
+++-------------------------------------
> src/ntf/ntfimcnd/ntfimcn_main.c | 18 ------
> 2 files changed, 8 insertions(+), 146 deletions(-)
>
> diff --git a/src/ntf/ntfd/ntfs_imcnutil.c b/src/ntf/ntfd/ntfs_imcnutil.c
> index 00c2c0039..8e9db29ea 100644
> --- a/src/ntf/ntfd/ntfs_imcnutil.c
> +++ b/src/ntf/ntfd/ntfs_imcnutil.c
> @@ -55,27 +55,19 @@ pthread_mutex_t ntfimcn_mutex;
> * Kill the osafntfimcn child process using previously saved Pid
> * Error handled
> *
> - * @param signal A valid signal
> - * @return -1 on error (kill failed
> */
> -static void kill_imcnproc(int signal)
> +static void kill_imcnproc()
> {
> - int rc = 0;
> -
> - rc = kill(ipar.pid, signal);
> + LOG_NO("%s: Terminating osafntfimcnd (%d) process",
> + __FUNCTION__, ipar.pid);
> + int rc = kill(ipar.pid, SIGKILL);
> if (rc == -1) {
> if (errno == EPERM) {
> - /* We don't have permission to kill the process! */
> + // We don't have permission to kill the process!
> LOG_ER(
> "Child process osafntfimcn could not be killed:
%s",
> strerror(errno));
> - } else if (errno == ESRCH) {
> - /* The process was not found */
> - LOG_NO(
> - "Child process osafntfimcn could not be killed:
%s",
> - strerror(errno));
> - } else {
> - /* EINVAL Invalid or unsupported signal number */
> + } else if (errno != ESRCH) {
[Vu] Use "else {" instead (without if) in order to cover all error cases?
> LOG_ER("%s Fatal error when killing %s",
> __FUNCTION__,
> strerror(errno));
> osaf_abort(0);
> @@ -83,115 +75,6 @@ static void kill_imcnproc(int signal)
> }
> }
>
> -/**
> - * Timed out wait for the imcnproc to terminate.
> - *
> - * @return -1 If wait timed out
> - */
> -static int wait_imcnproc_termination(void)
> -{
> - struct timespec wait_poll;
> - pid_t rc_pid = (pid_t)0;
> - int retry_cnt = 0;
> - const int max_retry = 100;
> - const uint64_t wait_poll_ms = 100;
> - int status = 0;
> - int rc = 0;
> -
> - TRACE_ENTER();
> - osaf_millis_to_timespec(wait_poll_ms, &wait_poll);
> -
> - /* Wait for child process to terminate */
> - retry_cnt = 0;
> - do {
> - rc_pid = waitpid(ipar.pid, &status, WNOHANG);
> - if (rc_pid == -1) {
> - if (errno == EINTR) {
> - TRACE("\tGot EINTR continue waiting");
> - rc_pid = (pid_t)0;
> - } else if (errno == ECHILD) {
> - TRACE("\t Done, the process is terminated");
> - rc = 0;
> - break;
> - } else {
> - /* EINVAL Invalid option */
> - LOG_ER("%s Internal error %s", __FUNCTION__,
> - strerror(errno));
> - osaf_abort(0);
> - }
> - }
> -
> - /* No status available. Wait and try again */
> - retry_cnt++;
> - if (retry_cnt <= max_retry) {
> - osaf_nanosleep(&wait_poll);
> - } else {
> - TRACE("\tTermination timeout");
> - rc = -1;
> - break;
> - }
> - } while (rc_pid == 0);
> -
> - TRACE_LEAVE2("rc = %d, retry_cnt = %d", rc, retry_cnt);
> - return rc;
> -}
> -
> -/**
> - * Wait for imcn process to exit.
> - * 1.Send SIGTERM to the imcn process
> - * - Wait for process to terminate
> - * ~ Prevents "zombie" process
> - * ~ Guaranty that process is terminated before return
> - * If the process does not terminate after a timeout the
> - * termination escalates with the following steps:
> - * 2. Abort signal is sent to imcn process
> - * Wait for termination. If fail do next step.
> - * 3. Send SIGKILL
> - * Wait for termination. If no termination is detected, ignore and
> - * continue NTF. Something is wrong and we may end up with a 'zombie'
> - * process but there is nothing more to do except aborting NTF
causing a
> - * node restart.
> - *
> - * Also handle the case that the process already is killed before
> - * this function is called.
> - *
> - */
> -static void timedwait_imcn_exit(void)
> -{
> - int rc = 0;
> - TRACE_ENTER();
> -
> - /* Send SIGTERM for normal termination */
> - kill_imcnproc(SIGTERM);
> - rc = wait_imcnproc_termination();
> - if (rc == 0) {
> - TRACE("\tImcn successfully terminated");
> - goto done;
> - }
> -
> - /* Normal termination has timed out. Escalate to step 2 */
> - TRACE("\tNormal termination failed. Escalate to abort");
> - kill_imcnproc(SIGABRT);
> - rc = wait_imcnproc_termination();
> - if (rc == 0) {
> - TRACE("\tImcn successfully aborted");
> - goto done;
> - }
> -
> - /* Abort has also timed out. Escalate to step 3 */
> - TRACE("\tNormal termination failed. Escalate to kill");
> - kill_imcnproc(SIGKILL);
> - rc = wait_imcnproc_termination();
> - if (rc == 0) {
> - TRACE("\tImcn successfully killed");
> - } else {
> - LOG_WA("The osafntfimcnd process could not be killed!");
> - }
> -
> -done:
> - TRACE_LEAVE();
> -}
> -
> static pid_t create_imcnprocess(SaAmfHAStateT ha_state)
> {
> char *start_args[3];
> @@ -329,9 +212,7 @@ void handle_state_ntfimcn(SaAmfHAStateT ha_state)
> osaf_mutex_lock_ordie(&ntfimcn_mutex);
> if (ha_state != ipar.ha_state) {
> ipar.ha_state = ha_state;
> - TRACE("%s: Terminating osafntfimcnd process",
> - __FUNCTION__);
> - timedwait_imcn_exit();
> + kill_imcnproc();
> } else {
> TRACE("%s: osafntfimcnd process not restarted. "
> "Already in correct state",
> @@ -361,8 +242,7 @@ int stop_ntfimcn(void)
> /* Kill ntfimcn */
> osaf_mutex_lock_ordie(&ntfimcn_mutex);
> ipar.ntfimcn_on = false;
> - TRACE("%s: Terminating osafntfimcnd process", __FUNCTION__);
> - timedwait_imcn_exit();
> + kill_imcnproc();
> osaf_mutex_unlock_ordie(&ntfimcn_mutex);
>
> /* Cancel the surveillance thread */
> diff --git a/src/ntf/ntfimcnd/ntfimcn_main.c
> b/src/ntf/ntfimcnd/ntfimcn_main.c
> index 51be3dce0..40da79c32 100644
> --- a/src/ntf/ntfimcnd/ntfimcn_main.c
> +++ b/src/ntf/ntfimcnd/ntfimcn_main.c
> @@ -78,18 +78,6 @@ static void sigusr2_handler(int sig)
> }
> }
>
> -/**
> - * TERM signal handler
> - *
> - * @param sig[in]
> - */
> -static void sigterm_handler(int sig)
> -{
> - (void)sig;
> - signal(SIGTERM, SIG_IGN);
> - _Exit(EXIT_SUCCESS);
> -}
> -
> /*
> * Exit if anything fails. This will cause ntfs to restart ntfimcn
> */
> @@ -99,12 +87,6 @@ int main(int argc, char **argv)
> const char *trace_label = "osafntfimcnd";
> SaAisErrorT ais_error = SA_AIS_OK;
>
> - // To make sure the SIGTERM is not lost during init phase
> - if (signal(SIGTERM, sigterm_handler) == SIG_ERR) {
> - LOG_ER("signal TERM failed: %s", strerror(errno));
> - _Exit(EXIT_FAILURE);
> - }
> -
> /*
> * Activate Log Trace
> */
> --
> 2.15.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
