Suggested by: delphij@
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| libexec/rc/rc.d/motd | ||
|---|---|---|
| 34 ↗ | (On Diff #58879) | Because this is a template, it do not really need the header anymore, because we prepend it anyway. This should be replaced with: sed '1{/^FreeBSD.*/{d;};};' "${COMPAT_MOTD}" > "${TEMPLATE}" |
| 39 ↗ | (On Diff #58879) | TEMPLATE permission should be set here (or after line 34). |
| 41 ↗ | (On Diff #58879) | 41-42: ln -sF "${TARGET}" "${COMPAT_MOTD}" would take care of the pre-existing file. |
| 45 ↗ | (On Diff #58879) | 45-47: If after the above dance the template still do not exist as a file, we should bail out (return) or just ignore. |
| 49 ↗ | (On Diff #58879) | 49-52 should be deleted. It's fine when ${TARGET} do not exist, we can generate its contents. |
| 56 ↗ | (On Diff #58879) | Note that since we now store template separately, this edit should only happen once. In other words, replace this with cat "${TEMPLATE}" >> ${T} |
| usr.bin/login/motd.template | ||
| 1 ↗ | (On Diff #58879) | This line should be deleted. |
Comment Actions
Sorry about the delay! I've been traveling and not very available. Thanks for the feedback, I will try to address it now.
| libexec/rc/rc.d/motd | ||
|---|---|---|
| 34 ↗ | (On Diff #58879) | Thanks, will fix. |
| 39 ↗ | (On Diff #58879) | Will fix, thanks. |
| 41 ↗ | (On Diff #58879) | -F or -f? Do we have any reason to believe /etc/motd is a directory? And if it is, that it is empty? Yes, -f takes care of the expected preexisting file. I guess ln -sf unconditionally unlinks and recreates the symlink inode every time, but we are still conditional on [ ! -f $TEMPLATE ], so probably the IO isn't a concern. Will plan to change to -f unless you feel strongly about -F. |
| 45 ↗ | (On Diff #58879) | Ok |
| 49 ↗ | (On Diff #58879) | Hm, what do you think about testing ${TARGET}/.. (i.e., "/var/run") for writability instead? if [ ! -w /var/run ] || [ -f "${TARGET}" -a ! -w "${TARGET}" ]; then
...OTOH, the test is inherited from the pretty ancient script; maybe we can assume we have a writable /var/run now. I am ok to delete it if you prefer. |
| 56 ↗ | (On Diff #58879) | Will fix. |
| usr.bin/login/motd.template | ||
| 1 ↗ | (On Diff #58879) | Will fix, thanks. |
Comment Actions
- Discard header from template (both from new install and compat import of existing motd)
- Explicitly set template file permission when created from legacy motd
- Simplify replacing preexisting non-symlink /etc/motd
- Correct writability test for generated TARGET
- Simplify construction of /var/run/motd from template with header discarded
Comment Actions
Thanks! Please see my comment inline.
I'd perfer -F be used (see reasoning inline; but I won't insist); the writeability check should be removed as install would give more meaningful messages.
| libexec/rc/rc.d/motd | ||
|---|---|---|
| 41 ↗ | (On Diff #58879) | I don't have strong opinion here but it was intentional that -F is used (reasoning below): When -f is used, and /etc/motd was a directory, and there is nothing preventing ln from creating /etc/motd/motd, it would silently succeed with unwanted result; on the other hand with -F, ln would make its best effort (delete empty directory /etc/motd or pre-existing /etc/motd file and create the symlink), and fail when it can't do it, so overall it's slightly better than -f. |
| 49 ↗ | (On Diff #58879) | Yeah let's just remove this portion of code. The install below would emit more meaningful error messages, and this test is not really accurate anyway. |