-
Notifications
You must be signed in to change notification settings - Fork 143
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
Add boot counting #24
Conversation
util/grub.d/00_menu_auto_hide.in
Outdated
save_env boot_counter | ||
fi | ||
fi | ||
|
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 don't think this belong to 00_menu_auto_hide.in
, since this an independent feature I would add it in a new script file (i.e: 01_fallback_support
).
That way we can for example ship the menu auto hide and fallback support independently depending on the Fedora variant that needs it, users can choose to disable one feature bit removing the script executable bit without affecting the other feature, etc.
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.
That way, both features couldn't be used at the same time. I'd like to have that option for workstations (even if it weren't active by default). I need the lines resetting and saving boot_success
from this script. maybe @jwrdegoede has an idea how to separate things cleanly here?
util/grub.d/00_menu_auto_hide.in
Outdated
if [ "\${boot_counter}" = "0" ]; then | ||
set default=1 | ||
set fallback_active=true | ||
save_env fallback_active |
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.
Probably you don't need this fallback_active
variable, I would just use the boot_counter
variable as an indication that the fallback entry was booted. For example you can set here boot_counter
to -1 or unset it so user-space knows that the default entry failed to boot.
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.
ack
util/grub.d/00_menu_auto_hide.in
Outdated
EOF | ||
|
||
# Disable / skip generating menu-auto-hide config parts on serial terminals | ||
for x in ${GRUB_TERMINAL_INPUT} ${GRUB_TERMINAL_OUTPUT}; do |
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.
Is this the distinction made between Server and Workstation editions?
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.
Nice! Just some minor comments below.
util/grub.d/00_menu_auto_hide.in
Outdated
cat << EOF | ||
# Boot Counting | ||
if [ "\${boot_counter}" ]; then | ||
if [ "\${boot_success}" = "0"]; then |
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.
Minor: can we just collapse these two as:
if [ "\${boot_counter}" -a "\${boot_success}" = "0" ]; then
?
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.
@jlebon I don't think we can since boot_counter
not set is used as an indication that fallback is disabled on that system.
Otherwise you would need to have in the case statement:
elif [ "\${boot_counter}" ]; then
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.
Hmm, I'm not sure I understand this. Isn't
if A then
if B then
...
fi
fi
logically equivalent to:
if A and B then
...
fi
?
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.
@jlebon yes it is, but you are missing the else part in your example:
if A then
if B then
...
else
...
fi
fi
if A and B then
...
else
...
fi
The else statement will be executed even if not A
so then you need:
if A and B then
...
elif not A then
...
fi
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.
There is no else
on the first two if-statements though, only the third. :)
Anyway, it's just a minor nit; not really worth discussing it more than we already have.
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.
@jlebon sorry, I misread and thought you were talking about the second if-statement. Yes, for the first one you are correct! cc @LorbusChris
util/grub.d/00_menu_auto_hide.in
Outdated
save_env boot_counter | ||
else | ||
set boot_counter=$((\${boot_counter}-1)) | ||
save_env boot_counter |
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.
Minor: we can just factor out the save_env boot_counter
outside the if-statement, right?
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.
@jlebon agreed.
1aa88cc
to
bbc6992
Compare
Changes since v1:
|
You are missing this bit though, it has to be on a preparatory commit before adding |
db8cd3e
to
a4eb01c
Compare
done @martinezjavier @jlebon |
@LorbusChris the second patch looks good to me now. Could you please add a commit message in the same commit explaining why this rename is needed. Also, could you please rebase on top of the latest fedora-29 branch? |
0b72724
to
0b54f71
Compare
@martinezjavier please re-check =) Edit: Changes v2 -> v3: |
0b54f71
to
7fbd610
Compare
This is necessary to accommodate the fallback counting script which needs to run before this one because the menu auto hide script sets boot_success = 0, which will be used by the boot counting script
Adds 01_fallback_counting.in script to support boot counting before falling back to the previous menu entry automatically
7fbd610
to
3431894
Compare
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.
Looks good to me now.
Introduces a
boot_counter
env var that counts down boot attempts to 0.If 0 is reached it will set
default=1
to boot the next older entry once (I don't need to setboot_once
here for this, do I?)Also introduces a
fallback_active
var that is set totrue
in this case, to make it easy to determine from userspace that this is a fallback boot.As I need the automatic
boot_success
reset mechanism in all cases, I put this in00_menu_auto_hide.in
for now.Use case: https://github.com/LorbusChris/greenboot/pull/16
RFC please
@jwrdegoede @dustymabe @jlebon @nullr0ute