Hi Martin, My patch have tested but your patch seems to be more gracefully. May be there are some small issues if I am not wrong.Thanks.
On 2018/1/30 22:18, Martin Wilck wrote: > multipathd shouldn't try to service any more client connections > when it receives an exit signal. Moreover, ppoll() can return > success even if signals occured. So check for reconfigure or > log_reset signals after handling pending client requests. > > Based on an analysis by Chongyun Wu. > > Reported-by: Chongyun Wu <[email protected]> > Signed-off-by: Martin Wilck <[email protected]> > --- > multipathd/main.c | 6 ++++-- > multipathd/main.h | 2 +- > multipathd/uxlsnr.c | 7 +++++-- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 27cf234623d0..8e96f5dd2d7f 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2184,12 +2184,15 @@ signal_set(int signo, void (*func) (int)) > } > > void > -handle_signals(void) > +handle_signals(bool nonfatal) > { > if (exit_sig) { > condlog(2, "exit (signal)"); > + exit_sig = 0; If we received a exit signal should we just escape all below commands process to avoid the race condition between thread cancellation and commands processing? so maybe flag exit_sig keep to 1 here? > exit_daemon(); > } > + if (!nonfatal) > + return; > if (reconfig_sig) { > condlog(2, "reconfigure (signal)"); > set_config_state(DAEMON_CONFIGURE); > @@ -2200,7 +2203,6 @@ handle_signals(void) > log_reset("multipathd"); > pthread_mutex_unlock(&logq_lock); > } > - exit_sig = 0; > reconfig_sig = 0; > log_reset_sig = 0; > } > diff --git a/multipathd/main.h b/multipathd/main.h > index ededdaba32fe..e8140feaf291 100644 > --- a/multipathd/main.h > +++ b/multipathd/main.h > @@ -38,6 +38,6 @@ int mpath_pr_event_handle(struct path *pp); > void * mpath_pr_event_handler_fn (void * ); > int update_map_pr(struct multipath *mpp); > void * mpath_pr_event_handler_fn (void * pathp ); > -void handle_signals(void); > +void handle_signals(bool); > > #endif /* MAIN_H */ > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c > index 98ac25a68c43..dc116cf2515b 100644 > --- a/multipathd/uxlsnr.c > +++ b/multipathd/uxlsnr.c > @@ -221,9 +221,10 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, > void * trigger_data) > /* most of our life is spent in this call */ > poll_count = ppoll(polls, i, &sleep_time, &mask); > > + handle_signals(false); Is it better to add and check the return value from handle_signals()? If return true means this a exit signal should run continue to escape all below processing. If not doing that, we may need to add pthread_cleanup_push(cleanup_lock, &client_lock) before pthread_mutex_lock(&client_lock) in below command processing to avid dead lock which happen when commands processing code just have the lock and cancellation works to call uxsock_cleanup hook to get the lock and the commands processing code have no chance to release the lock. > if (poll_count == -1) { > if (errno == EINTR) { > - handle_signals(); > + handle_signals(true); > continue; > } > > @@ -233,7 +234,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, > void * trigger_data) > } > > if (poll_count == 0) { > - handle_signals(); > + handle_signals(true); > continue; > } > > @@ -292,6 +293,8 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, > void * trigger_data) > FREE(inbuf); > } > } > + /* see if we got a non-fatal signal */ > + handle_signals(true); above code maybe not needed. Because it's safe to make sure no commands be processed then to handle nonefatal signal, like code "if(poll_count ==0){...}" do. If here to call hanle_signals(true) again, may facing race condition between signals processing and command processing. That maybe not so safe. And the handle_signals have been called above. Is there a special reason for this which I missed? > > /* see if we got a new client */ > if (polls[0].revents & POLLIN) { > Chongyun Wu -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
