Patchwork [3,of,3] chg: forward SIGINT, SIGHUP to process group

login
register
mail settings
Submitter Jun Wu
Date July 17, 2016, 10:39 p.m.
Message ID <1468794496-sup-9542@x1c>
Download mbox | patch
Permalink /patch/15928/
State Not Applicable
Headers show

Comments

Jun Wu - July 17, 2016, 10:39 p.m.
Excerpts from Jun Wu's message of 2016-07-17 23:09:43 +0100:
> -static void setupsignalhandler(pid_t pid)
> +static void setupsignalhandler(const hgclient_t *hgc)
>  {
> -    if (pid <= 0)
> +    peerpgid = hgc_peerpgid(hgc);
> +    peerpid = hgc_peerpid(hgc);

I noticed this changed behavior a bit as I pursued short code: the second
setupsignalhandler call (if exists) becomes unsafe and the if condition is
not quite right. It's probably better not to save lines:
Yuya Nishihara - July 18, 2016, 10:06 a.m.
On Sun, 17 Jul 2016 23:39:54 +0100, Jun Wu wrote:
> Excerpts from Jun Wu's message of 2016-07-17 23:09:43 +0100:
> > -static void setupsignalhandler(pid_t pid)
> > +static void setupsignalhandler(const hgclient_t *hgc)
> >  {
> > -    if (pid <= 0)
> > +    peerpgid = hgc_peerpgid(hgc);
> > +    peerpid = hgc_peerpid(hgc);  
> 
> I noticed this changed behavior a bit as I pursued short code: the second
> setupsignalhandler call (if exists) becomes unsafe and the if condition is
> not quite right. It's probably better not to save lines:
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -404,10 +404,14 @@ static void handlechildsignal(int sig UN
>  
>  static void setupsignalhandler(const hgclient_t *hgc)
>  {
> -       peerpgid = hgc_peerpgid(hgc);
> -       peerpid = hgc_peerpid(hgc);
> -       if (peerpgid <= 0 && peerpid <= 0)
> +       pid_t pid = hgc_peerpid(hgc);
> +       if (pid <= 0)
>                 return;
> +       pid_t pgid = hgc_peerpgid(hgc);
> +       if (pgid < 0)
> +               pgid = 0;
> +       peerpid = pid;
> +       peerpgid = pgid;

Yeah, something like that. Also, we should check pgid > 1 because kill(-1, sig)
would kill all user processes.

 static void forwardsignaltogroup(int sig)
 {
+	assert(peerpid > 0);
 	/* prefer kill(-pgid, sig) but use pid if pgid is unavailable */
-	pid_t killpid = peerpgid ? -peerpgid : peerpid;
+	pid_t killpid = (peerpgid > 1) ? -peerpgid : peerpid;
 	if (kill(killpid, sig) < 0)
 		abortmsgerrno("cannot kill %d", killpid);
 	debugmsg("forward signal %d to %d", sig, killpid);

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -404,10 +404,14 @@  static void handlechildsignal(int sig UN
 
 static void setupsignalhandler(const hgclient_t *hgc)
 {
-       peerpgid = hgc_peerpgid(hgc);
-       peerpid = hgc_peerpid(hgc);
-       if (peerpgid <= 0 && peerpid <= 0)
+       pid_t pid = hgc_peerpid(hgc);
+       if (pid <= 0)
                return;
+       pid_t pgid = hgc_peerpgid(hgc);
+       if (pgid < 0)
+               pgid = 0;
+       peerpid = pid;
+       peerpgid = pgid;
 
        struct sigaction sa;
        memset(&sa, 0, sizeof(sa));