Hi!

----

Below are a could of questions about |job_kill()| in src/cmd/ksh93/sh/jobs.c:
-- snip --
  1158  /*
  1159   * Kill a job or process
  1160   */
  1161  
  1162  int job_kill(register struct process *pw,register int sig)
  1163  {
  1164          Shell_t *shp;
  1165          register pid_t pid;
  1166          register int r;
  1167          const char *msg;
  1168          int qflag = sig&JOB_QFLAG;
  1169  #ifdef SIGTSTP
  1170          int stopsig;

AFAIK this should be |bool| and not |int| ...

  1171  #endif
  1172  #if _lib_sigqueue
  1173          union sigval sig_val;
  1174  #else
  1175          int sig_val = 0;
  1176  #endif
  1177          if(pw==0)
  1178                  goto error;
  1179          shp = pw->p_shp;
  1180  #if _lib_sigqueue
  1181          sig_val.sival_ptr = shp->sigmsg;
  1182  #endif

Erm... why is |sival_ptr| used and not |sival_int| ? The issue is that
on systems which have both 32bit and 64bit processes signaling a 32bit
process from a 64bit one may discard part of the message depending on
byte order of the pointer...
... the other issue is that the union shold be cleared by |memset()|
to prevent issues with uninitalised data being passed around.

  1183          sig &= ~JOB_QFLAG;
  1184  #ifdef SIGTSTP
  1185          stopsig = 
(sig==SIGSTOP||sig==SIGTSTP||sig==SIGTTIN||sig==SIGTTOU);
  1186  #else
  1187  #       define stopsig  1
  1188  #endif  /* SIGTSTP */
  1189          job_lock();
  1190          errno = ECHILD;
  1191          pid = pw->p_pid;
  1192  #if SHOPT_COSHELL
  1193          if(pw->p_cojob)
  1194                  r = cokill(pw->p_cojob->coshell,pw->p_cojob,sig);
  1195          else
  1196  #endif /* SHOPT_COSHELL */
  1197          if(by_number)

Why is |by_number| a global variable and not passed as argument ?

  1198          {
  1199                  if(pid==0 && job.jobcontrol)
  1200                          r = job_walk(shp,outfile, job_kill,sig, 
(char**)0);
  1201  #ifdef SIGTSTP
  1202                  if(sig==SIGSTOP && pid==shp->gd->pid && 
shp->gd->ppid==1)
  1203                  {
  1204                          /* can't stop login shell */
  1205                          errno = EPERM;
  1206                          r = -1;
  1207                  }
  1208                  else
  1209                  {
  1210                          if(pid>=0)
  1211                          {
  1212                                  if(qflag)
  1213                                  {
  1214                                          if(pid==0)
  1215                                                  goto no_sigqueue;
  1216                                          r = sigqueue(pid,sig,sig_val);
  1217                                  }
  1218                                  else
  1219                                          r = kill(pid,sig);
  1220                                  if(r>=0 && !stopsig)
  1221                                  {
  1222                                          if(pw->p_flag&P_STOPPED)
  1223                                                  pw->p_flag &= 
~(P_STOPPED|P_SIGNALLED);
  1224                                          if(sig)
  1225                                                  kill(pid,SIGCONT);
  1226                                  }

See http://lists.research.att.com/pipermail/ast-developers/2013q3/002782.html
for my proposed change to only send SIGCONT to processes we a) own
(e.g. are child processes) and b) are in the stopped state (and even
then (my patch doesn't do this yet) the functionality to wake a child
process each time a signal is send to it should be optional and not
"forced on").
-- snip --
--- src/cmd/ksh93/sh/jobs.c      2013-06-14 22:55:44.000000000 +0200
+++ src/cmd/ksh93/sh/jobs.c    2013-07-06 00:47:08.867238765 +0200
@@ -1219,10 +1219,11 @@
                                        r = kill(pid,sig);
                                if(r>=0 && !stopsig)
                                {
-                                       if(pw->p_flag&P_STOPPED)
+                                       if(sig && pw->p_flag&P_STOPPED)
+                                       {
                                                pw->p_flag &=
~(P_STOPPED|P_SIGNALLED);
-                                       if(sig)
                                                kill(pid,SIGCONT);
+                                       }
                                }
                        }
-- snip --

  1227                          }
  1228                          else
  1229                          {
  1230                                  if(qflag)
  1231                                          goto no_sigqueue;
  1232                                  if((r = killpg(-pid,sig))>=0 && 
!stopsig)
  1233                                  {
  1234                                          
job_unstop(job_bypid(pw->p_pid));
  1235                                          if(sig)
  1236                                                  killpg(-pid,SIGCONT);

Erm...
1. ... why is |killpg(-pid,SIGCONT);| issued here explicitly ?
|job_unstop()| did already send SIGCONT to the process group if there
are any child processes in the stopped state...
2. ... The |stopsig| functionality is nowhere documented (it's
|stopsig = (sig==SIGSTOP||sig==SIGTSTP||sig==SIGTTIN||sig==SIGTTOU);|
we check for...) ... ;-(

  1237                                  }
  1238                          }
  1239                  }
  1240  #else
  1241                  if(pid>=0)
  1242                  {
  1243                          if(qflag)
  1244                                  r = sigqueue(pid,sig,sig_val);
  1245                          else
  1246                                  r = kill(pid,sig);
  1247                  }
  1248                  else
  1249                  {
  1250                          if(qflag)
  1251                                  goto no_sigqueue;
  1252                          r = killpg(-pid,sig);
  1253                  }
  1254  #endif  /* SIGTSTP */
  1255          }
  1256          else
  1257          {
  1258                  if(qflag)
  1259                          goto no_sigqueue;
  1260                  if(pid = pw->p_pgrp)
  1261                  {
  1262                          r = killpg(pid,sig);
  1263  #ifdef SIGTSTP
  1264                          if(r>=0 && (sig==SIGHUP||sig==SIGTERM || 
sig==SIGCONT))
  1265                                  job_unstop(pw);
  1266  #endif  /* SIGTSTP */
  1267                          if(r>=0)
  1268                                  sh_delay(.05);

Why is |sh_delay()| called in this case ?

  1269                  }
  1270                  while(pw && pw->p_pgrp==0 && (r=kill(pw->p_pid,sig))>=0)
  1271                  {
  1272  #ifdef SIGTSTP
  1273                          if(sig==SIGHUP || sig==SIGTERM)
  1274                                  kill(pw->p_pid,SIGCONT);

Ok... this appears to be the behaviour documented in
http://www2.research.att.com/sw/download/man/man1/ksh88.html (but
without a test whether the child is in the stopped state):
-- snip --
If the signal being sent is TERM (terminate) or HUP (hangup), then the
job or process will be sent a CONT (continue) signal if it is stopped.
-- snip --

  1275  #endif  /* SIGTSTP */
  1276                          pw = pw->p_nxtproc;
  1277                  }
  1278          }
  1279          if(r<0 && job_string)
  1280          {
  1281          error:
  1282                  if(pw && by_number)
  1283                          msg = sh_translate(e_no_proc);
  1284                  else
  1285                          msg = sh_translate(e_no_job);
  1286                  if(errno == EPERM)
  1287                          msg = sh_translate(e_access);
  1288                  sfprintf(sfstderr,"kill: %s: %s\n",job_string, msg);
  1289                  r = 2;
  1290          }
  1291          sh_delay(.001);

Why is |sh_delay()| called in this case ?

  1292          job_unlock();
  1293          return(r);
  1294  no_sigqueue:
  1295          msg = sh_translate("queued signals require positive pids");
  1296          sfprintf(sfstderr,"kill: %s: %s\n",job_string, msg);
  1297          return(2);
  1298  }
-- snip --

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) [email protected]
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)
_______________________________________________
ast-developers mailing list
[email protected]
http://lists.research.att.com/mailman/listinfo/ast-developers

Reply via email to