Fix rc.log missing shutdown messages in some cases.#980
Fix rc.log missing shutdown messages in some cases.#980e-pirate wants to merge 1 commit intoOpenRC:masterfrom
Conversation
file system by preventing localmount from unmouting this file system to early keeping rc.log file writible. Closes: OpenRC#977
|
|
||
| if yesno ${rc_logger:-NO}; then | ||
| [ ! -f "${rc_log_path}" ] && touch "${rc_log_path}" | ||
| local rc_log_mntpnt=$(stat --format %m "${rc_log_path}") |
There was a problem hiding this comment.
Is this portable? I haven't checked but mountinfo may have a way to return the mountpoint.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Right, I don't mean portable as in POSIX, I mean in terms of whether BSDs and busybox/alpine support it or not.
There was a problem hiding this comment.
Is this portable? I haven't checked but
mountinfomay 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
-cbut I can't find%mthere, 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.
There was a problem hiding this comment.
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.
| done | ||
|
|
||
| if yesno ${rc_logger:-NO}; then | ||
| [ ! -f "${rc_log_path}" ] && touch "${rc_log_path}" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How do you like this implementation?
[ -z ${rc_log_path+x} ] && rc_log_path="/var/log/rc.log" just before checking for file existence?
There was a problem hiding this comment.
you can instead, [ -f "${rc_log_path:=/var/log/rc.log}" ] to both check and assign if unset at once
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
then check both, yesno "$rc_logger" && [ -f ... ]
|
OK. this is a little bit more difficult as I expected. As expected. |
3 participants
Fix rc.log missing shutdown messages if located on separate (non-root) file system by preventing localmount from unmouting this file system to early keeping rc.log file writible.
Closes: #977