-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Try harder to become root and speed up by skipping unnecessary sudo's #608
base: master
Are you sure you want to change the base?
Conversation
I don't think performance matters for shell-scripts (they have no (JIT-)compiler and start new processes for everything they do). Anyway it allows to run the script on systems without sudo / su.
To quote
So it just searches for it in
It's not listed in
It is deprecated?
We could also check |
I agree with performance not being important. Still, low-importance improvements are also improvements. Although every call to a binary (e.g.
It seems that i was a bit to hasty with my conclusion. I tried my code on the tiniest "system" i had (a alpine docker container that barely has any binaries) and because it was present i incorrectly assumed it was a builtin. While writing this I'm suddenly thinking about a better way to check if
True, officially it's not deprecated.
The name vaguely rings a bell, but I've never used it. I'll take a look at it and use it in a 3rd commit if it it's a extra feature. |
If you want to only use builtins (is echo always a builtin?): if ! command -v sudo >&-; then
echo "no sudo"
fi |
Changes after this commit:
I've tested the returncode Note: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
sudo $0 && exit 0 | ||
if [ $? -eq 127 ] ; then | ||
echo "I can't find 'sudo', I'll try to use 'su' to become root." | ||
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tab -> 8 spaces
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)" | |
echo "(Remember that 'su' is considerd deprecated (it needs a root password), I strongly recommend installing 'sudo'.)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that i wrote considerd instead of considered . I'll fix both things in the next commit (after taking a look at pkexec
and including it if we can use it)
(Also fixes typo 'considerEd' typo and incorrect indenting
According to it's man-page We could extend the code even more by:
But according to me that's overkill, certainly because unless you have a really strange configuration, you will see a 'command-not-found'-type of error anyway and it will cause running |
Hi everyone, this looks marvelous. About your "We could extend the code [...]" part, I agree that it's overkill. People who manage "strange configuration" should be able to handle such thing. Anything to add @rusty-snake ? cc @mitchellkrogza. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything to add @rusty-snake ?
Exit-codes don't work like exceptions. The don't tell you the reason why some thing failed. The don't even tell you if something failed. IMHO it would be better to use command
(or which
) to check if sudo/su/pkexec are present.
Furthermore this has duplicated and nested code. Here are two ideas to improve this.
become_root() {
if command -v "$1" >/dev/null; then
"$@" && exit 0
echo "Something went wrong with trying to use '$1' to run this script" > /dev/stderr
else
return 1
fi
}
echo "This script needs to be ran as root, let's switch to root:" > /dev/stderr
become_root sudo "$0" && exit 0
echo "I can't find 'sudo', I'll try to use 'su' to become root." > /dev/stderr
echo "(Remember that 'su' is considered deprecated (it needs a root password), I strongly recommend installing 'sudo'.)" > /dev/stderr
become_root su -c "$0" && exit 0
echo "I also can't find 'su', I'll try to use 'pkexec' to run this as root..." > /dev/stderr
become_root pkexec "$0" && exit 0
echo "I can't give you authorization to run this as root, you'll need to find another way to become root..." > /dev/stderr
exit 1
echo "This script needs to be ran as root, let's switch to root:" > /dev/stderr
if command -v sudo >/dev/null; then
sudo "$0" && exit 0
echo "Something went wrong with trying to use 'sudo' to run this script" > /dev/stderr
exit 1
elif command -v su >/dev/null; then
su -c "$0"
elif command -v pkexec >/dev/null; then
pkexec "$0" && exit 0
echo "Something went wrong with trying to use 'pkexec' to run this script." > /dev/stderr
exit 1
else
echo "I can't give you authorization to run this as root, you'll need to find another way to become root..." > /dev/stderr
exit 1
fi
Great. Thanks for your input and time @rusty-snake. This PR will stay a draft until the recommendation of @rusty-snake are applied. Stay safe and healthy. |
All other scripts in
Installer-Linux
can be improved in the same way as below, but I'm not spending time on it if this doesn't get merged...This PR:
sudo
anymore if you are already rootsudo
is located instead of assuming it is in$PATH
(it useswhich
instead ofenv
for this becauseenv
is a binary that isn't necessarily available.which
on the other hand is also a shell-buildin that even shells as tiny as busybox have)su
if you don't havesudo
and warns you that this is deprecated (also not assuming it's in$PATH
)*The 'real' part of the script hasn't been improved yet, that's for another PR. So if one these commands fail you will obviously not see the correct reason yet