Page MenuHomeFreeBSD

motd: Generate from template to /var/run

Authored by cem on Jun 21 2019, 11:27 PM.



Suggested by: delphij@

Test Plan

I have not tested it yet.

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

  • Fix a typo in rc.d/motd
  • Update login(1) and motd.5
bcr added a subscriber: bcr.

OK for the manpage changes.

This revision is now accepted and ready to land.Jun 24 2019, 9:49 AM
delphij requested changes to this revision.Jun 25 2019, 4:37 PM
delphij added inline comments.
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}

1 ↗(On Diff #58879)

This line should be deleted.

This revision now requires changes to proceed.Jun 25 2019, 4:37 PM
cem planned changes to this revision.Jul 19 2019, 6:20 PM

Sorry about the delay! I've been traveling and not very available. Thanks for the feedback, I will try to address it now.

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)


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.

1 ↗(On Diff #58879)

Will fix, thanks.

cem marked 7 inline comments as done.
  • 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

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.

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.

cem marked an inline comment as done.Jul 19 2019, 11:33 PM
cem added inline comments.
41 ↗(On Diff #58879)

Ok, will change.

49 ↗(On Diff #58879)

works for me!

cem marked an inline comment as done.
  • -F
  • Drop not useful TARGET writability test
cem marked an inline comment as done.Jul 19 2019, 11:35 PM
This revision is now accepted and ready to land.Jul 20 2019, 1:43 AM
This revision was automatically updated to reflect the committed changes.