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
system-update-generator: warn if the command line blocks updates #5173
Conversation
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.
LGTM, just some nitpicks to trim the code a bit. But good to merge if you don't care. Thanks!
|
||
r = proc_cmdline_parse(parse_proc_cmdline_item, NULL, 0); | ||
if (r < 0) | ||
log_warning_errno(r, "Failed to parse kernel command line, ignoring: %m"); |
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.
nitpick: No need to have r
here, and since the entire function is so small and only being used once, it might be easier to just fold it into main()
below?
|
||
target = runlevel_to_target(key); | ||
if (target) | ||
log_warning("Offline system update overriden by runlevel \"%s\" on the kernel command line", key); |
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.
No need to save target
here, just `if (!value && runlevel_to_target(key) != NULL) { warn }' would be more succinct.
If "3", "5", "systemd.unit=", or similar are present on the kernel command line, the system will not enter into offline update. This behaviour is in line with the general logic that configuration on the kernel command line has higher priority than the configuration on disk, but is rather surprising. Emit a warning to help users diagnose the situation. https://bugzilla.redhat.com/show_bug.cgi?id=1405439#c4
aff8c8d
to
e843722
Compare
Yep, much nicer this way. Thanks for the review. Updated with changes. |
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.
Beautiful! ☺
If "3", "5", "systemd.unit=", or similar are present on the kernel command line,
the system will not enter into offline update. This behaviour is in line with the
general logic that configuration on the kernel command line has higher priority
than the configuration on disk, but is rather surprising. Emit a warning to help
users diagnose the situation.
https://bugzilla.redhat.com/show_bug.cgi?id=1405439#c4