[ast-developers] Questions about |job_kill()| in src/cmd/ksh93/sh/jobs.c ...

[Roland Mainz]

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