menu: exec the shell instead of running it as a child - #3775
alanhc wants to merge 1 commit into
Conversation
Sorry, something went wrong.
StartShell ran /bin/defaultsh as a child of the boot command, so boot
stayed alive underneath the shell and kept whatever it held open, such
as the loop devices for ISOs. Leaving the shell also made Exec return
nil, which bootcmd treats as fatal ("Kexec should have returned an
error or not returned at all").
Replace the process with unix.Exec instead, as the issue suggests.
StartShell then behaves like the other entries: Exec either fails or
does not return. Mod is still applied to build the path, arguments and
environment; its SysProcAttr settings no longer have any effect, since
no new process is started. Nothing in the tree sets Mod.
With boot as the uinit, before:
$ echo PARENT=$(cat /proc/$PPID/comm) SELF=$(cat /proc/$$/comm)
PARENT=uinit SELF=defaultsh
$ exit
Kexec should have returned an error or not returned at all.
after:
PARENT=init SELF=defaultsh
$ exit
$
Fixes u-root#3579
Signed-off-by: Hung-Chun Tseng <alan.tseng.cs@gmail.com>
Codecov Report❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## main #3775 +/- ##
==========================================
- Coverage 61.46% 61.32% -0.14%
==========================================
Files 648 648
Lines 45716 45719 +3
==========================================
- Hits 28099 28039 -60
- Misses 17617 17680 +63
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
|
This change kind of violates the intent of starting a shell. A common use of a shell is to enable the user to set things up, return to the boot command, and try again. With this change, that workflow is impossible. I am guessing you were exec'ing a shell, then running kexec, and not wanting the return? Also ... the comment about holding ISOs open. Is this after a mount? Why are ISOs left open after a mount? I'd like to understand that comment better. |
Sorry, something went wrong.
|
Fair point, and no, I wasn't exec'ing a shell to run kexec. I picked this up from #3579 and went with the approach suggested there. Today, though, that workflow doesn't work either: when the shell exits, StartShell.Exec returns nil and bootcmd hits log.Fatalf("Kexec should have returned an error or not returned at all"), so boot dies instead of going back to the menu. I hit that in QEMU with boot as the uinit (details in the PR description). On the ISOs: pkg/boot/iso loop-mounts them with AUTOCLEAR, and bootcmd unmounts with MNT_DETACH before Exec, so the loop devices only go away once nothing references the mounts. That was the reporter's observation in #3579; I haven't reproduced the ISO case myself. Would you rather have StartShell return to the menu when the shell exits? That would mean not unmounting before running the shell (or rescanning after), so I'd rework this PR along those lines. Happy to close this one if you prefer. |
Sorry, something went wrong.
Fixes #3579.
StartShell ran /bin/defaultsh as a child of the boot command, so boot stayed alive underneath the shell and kept whatever it held open, such as the loop devices for ISOs. Leaving the shell also made Exec return nil, which bootcmd treats as fatal ("Kexec should have returned an error or not returned at all").
This replaces the process with unix.Exec, as suggested in the issue. StartShell then behaves like the other entries: Exec either fails or does not return.
One trade-off: Mod is still applied to build the path, arguments and environment, but its SysProcAttr settings (e.g. Setsid/Setctty from WithTTYControl) no longer have any effect, since no new process is started. Nothing in the tree sets Mod.
Tested in QEMU (amd64) with init + boot as the uinit and gosh as defaultsh, choosing "Enter a LinuxBoot shell".
Before:
After:
I did not add a test, since unix.Exec replaces the test binary itself. The check above was a throwaway integration test; happy to turn it into a real one under integration/generic-tests if that is wanted.