Re: [PATCH] multipathd: fix inverted signal blocking logic

From: Martin Wilck <mwilck@suse.com>
To: Bart Van Assche <Bart.VanAssche@wdc.com>,
	"bmarzins@redhat.com" <bmarzins@redhat.com>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>
Subject: Re: [PATCH] multipathd: fix inverted signal blocking logic
Date: Fri, 02 Mar 2018 23:15:09 +0100	[thread overview]
Message-ID: <1520028909.4169.87.camel@suse.com> (raw)
In-Reply-To: <1520026513.2855.30.camel@wdc.com>

On Fri, 2018-03-02 at 21:35 +0000, Bart Van Assche wrote:
> On Fri, 2018-03-02 at 22:18 +0100, Martin Wilck wrote:
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 61739ac6ea59..85ee9b713d75 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2270,10 +2270,13 @@ signal_init(void)
> >  {
> >  	sigset_t set;
> >  
> > -	sigemptyset(&set);
> > -	sigaddset(&set, SIGUSR2);
> > +	/* block all signals */
> > +	sigfillset(&set);
> > +	/* SIGPIPE occurs if logging fails */
> > +	sigdelset(&set, SIGPIPE);
> >  	pthread_sigmask(SIG_SETMASK, &set, NULL);
> 
> The modified code was introduced by commit 90dd424afa65 ("multipathd:
> fix SIGUSR2 handling"). This change looks fine to me.
> 
> > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > index 98ac25a68c43..a2ca36ba1653 100644
> > --- a/multipathd/uxlsnr.c
> > +++ b/multipathd/uxlsnr.c
> > @@ -170,11 +170,11 @@ void * uxsock_listen(uxsock_trigger_fn
> > uxsock_trigger, void * trigger_data)
> >  		condlog(0, "uxsock: failed to allocate poll fds");
> >  		return NULL;
> >  	}
> > -	sigemptyset(&mask);
> > -	sigaddset(&mask, SIGINT);
> > -	sigaddset(&mask, SIGTERM);
> > -	sigaddset(&mask, SIGHUP);
> > -	sigaddset(&mask, SIGUSR1);
> > +	sigfillset(&mask);
> > +	sigdelset(&mask, SIGINT);
> > +	sigdelset(&mask, SIGTERM);
> > +	sigdelset(&mask, SIGHUP);
> > +	sigdelset(&mask, SIGUSR1);
> >  	while (1) {
> >  		struct client *c, *tmp;
> >  		int i, poll_count, num_clients;
> 
> This change looks more complicated to me than necessary. Have you
> considered
> to pass an empty signal set as the fourth ppoll() argument?

An empty set would mean that no signal is blocked during ppoll().
Therefore e.g. SIGALRM would terminate multipathd if it arrives
during the ppoll (no handler set, and default action is "Term").

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel