-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Faulty logic for disabling job control in virtual subshells #89
Comments
I started a related Austin Group thread. Before applying a fix I want to await the outcome of that discussion. |
In the meantime, here is a patch to test... it should make both this bug and #86 go away. diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index 5b12f92..c0eb47b 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -567,14 +567,14 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
jmpval = sigsetjmp(buff.buff,0);
if(jmpval==0)
{
+ /* disable job control */
+ sp->jobcontrol = job.jobcontrol;
+ sp->monitor = (sh_isstate(SH_MONITOR)!=0);
+ job.jobcontrol = 0;
+ sh_offstate(SH_MONITOR);
if(comsub)
{
- /* disable job control */
shp->spid = 0;
- sp->jobcontrol = job.jobcontrol;
- sp->monitor = (sh_isstate(SH_MONITOR)!=0);
- job.jobcontrol=0;
- sh_offstate(SH_MONITOR);
sp->pipe = sp;
/* save sfstdout and status */
sp->saveout = sfswap(sfstdout,NIL(Sfio_t*));
@@ -626,14 +626,14 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
if(!shp->savesig)
shp->savesig = -1;
nv_restore(sp);
+ /* re-enable job control */
+ if(!sp->nofork)
+ sh_offstate(SH_NOFORK);
+ job.jobcontrol = sp->jobcontrol;
+ if(sp->monitor)
+ sh_onstate(SH_MONITOR);
if(comsub)
{
- /* re-enable job control */
- if(!sp->nofork)
- sh_offstate(SH_NOFORK);
- job.jobcontrol = sp->jobcontrol;
- if(sp->monitor)
- sh_onstate(SH_MONITOR);
if(sp->pipefd>=0)
{
/* sftmp() file has been returned into pipe */
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index a6ad507..58e979f 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1633,7 +1633,7 @@ int sh_exec(register const Shnode_t *t, int flags)
}
if(type&FAMP)
{
- if(sh_isstate(SH_PROFILE) || sh_isstate(SH_INTERACTIVE))
+ if(sh_isstate(SH_PROFILE) || (sh_isstate(SH_INTERACTIVE) && sh_isstate(SH_MONITOR)))
{
/* print job number */
#ifdef JOBS |
My earlier statement:
is wrong. Read Geoff Clare's responses in this Austin Group thread to learn why. So the patch above is wrong as well. |
Closing this as my assumption was wrong. Job control in scripts should be fixed though, see #119. |
Job control should be disabled in subshells as it makes no sense there; subshells are never interactive and job control only makes sense on interactive shells. However,for non-forking subshells, ksh currently only disables job control for command substitutions:ksh/src/cmd/ksh93/sh/subshell.c
Lines 570 to 577 in f207cd5
ksh/src/cmd/ksh93/sh/subshell.c
Lines 629 to 636 in f207cd5
This causes "interesting" behaviour, e.g.:
This wrongly prints a job number.
This waits for
sleep
to end, but should complain about no job control instead. However:So that's inconsisistent.
Related: #79, #86
Also:
This wrongly prints a job number after the prompt, i.e. the job number is printed from a background process! So the main shell forks as well as the background process. That's a separate bug, probably a parser bug causing a double fork, akin to the one I fixed in 1026006.
The text was updated successfully, but these errors were encountered: