Skip to content

Fix rc.log missing shutdown messages in some cases. #980

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.

Sign up for GitHub

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

Open
e-pirate wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −0
Open

Fix rc.log missing shutdown messages in some cases. #980

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions init.d/localmount.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ stop()
no_umounts_r="$no_umounts_r|$x"
done

if yesno ${rc_logger:-NO}; then
[ ! -f "${rc_log_path}" ] && touch "${rc_log_path}"
Copy link
Member

@navi-desu navi-desu Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the logger is on, we'll always have the file by this point, so we might as well just check for it's existence directly i think

also rc_log_path isn't always set, it defaults to /var/log/rc.log if unset

Copy link
Author

@e-pirate e-pirate Feb 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the logger is on, we'll always have the file by this point, so we might as well just check for it's existence directly i think

also rc_log_path isn't always set, it defaults to /var/log/rc.log if unset

It is not the case as far as I can see. The rc.log file may present and the path is set by user but user may not enable logging (or disable it) and we should not bother at all in that cases. I believe we should attempt doing just anything only in case user deliberately enabled logging regardless path is set. I have to investigate it rc_log_path is available if not deliberately set and how to handle this case.

Copy link
Author

@e-pirate e-pirate Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. that's pretty easy, just check if rc_log_path variable is set, if not, set to to default value = /var/log/rc.log, otherwise keep as is.

Copy link
Author

@e-pirate e-pirate Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you like this implementation?
[ -z ${rc_log_path+x} ] && rc_log_path="/var/log/rc.log" just before checking for file existence?

Copy link
Member

@navi-desu navi-desu Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can instead, [ -f "${rc_log_path:=/var/log/rc.log}" ] to both check and assign if unset at once

Copy link
Author

@e-pirate e-pirate Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navi-desu we also may not have the file with logging enabled. If you enable logging for the first time during system is already running you will have rc_logger set to YES, but no file. There will be no file until next complete reboot.

Copy link
Member

@navi-desu navi-desu Feb 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then check both, yesno "$rc_logger" && [ -f ... ]

local rc_log_mntpnt=$(stat --format %m "${rc_log_path}")
Copy link
Contributor

@N-R-K N-R-K Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this portable? I haven't checked but mountinfo may have a way to return the mountpoint.

Copy link
Member

@navi-desu navi-desu Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mountinfo does not, but adding such flag was what i was considering doing

don't think it can be done truly portably since posix doesn't specify mounting at all, so if stat -c %m exists on all our platforms, that's enough i think

Copy link
Contributor

@N-R-K N-R-K Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I don't mean portable as in POSIX, I mean in terms of whether BSDs and busybox/alpine support it or not.

Copy link
Author

@e-pirate e-pirate Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this portable? I haven't checked but mountinfo may have a way to return the mountpoint.

I also took care about this. stat is a part of coreutils and should be available, unlike mountpoint that is part of sys-apps/util-linux.

Copy link
Contributor

@N-R-K N-R-K Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but (GNU) coreutils isn't used everywhere. BSDs don't use it, and even some linux distros such as alpine doesn't use it.

Looking at busybox's stat, I see that it supports -c but I can't find %m there, so it's likely not supported.

Copy link
Author

@e-pirate e-pirate Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but (GNU) coreutils isn't used everywhere. BSDs don't use it, and even some linux distros such as alpine doesn't use it.

Looking at busybox's stat, I see that it supports -c but I can't find %m there, so it's likely not supported.

I will ask an actual BSD user. If not supported, we may put this section inside the next block that works only for Linux.

Copy link
Contributor

@N-R-K N-R-K Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may put this section inside the next block that works only for Linux.

Except that linux != GNU coreutils. Alpine is linux. Alpine uses openrc. But it does NOT use GNU coreutils, it uses busybox.

The linux specific section below works because it's doing linux specific file system things. It doesn't make assumption about userland. Assuming linux kernel == GNU coreutils is not correct.

no_umounts_r="$no_umounts_r|${rc_log_mntpnt}"
fi

if [ "$RC_UNAME" = Linux ]; then
no_umounts_r="$no_umounts_r|/proc|/proc/.*|/run|/sys|/sys/.*"
if [ -e "$rc_svcdir"/usr_premounted ]; then
Expand Down
Loading