Page MenuHomeFreeBSD

motd: Generate from template to /var/run
ClosedPublic

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

Details

Summary

Suggested by: delphij@

Test Plan

I have not tested it yet.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24976
Build 23697: arc lint + arc unit

Event Timeline

cem created this revision.Jun 21 2019, 11:27 PM
cem updated this revision to Diff 58879.Jun 21 2019, 11:37 PM
  • Fix a typo in rc.d/motd
  • Update login(1) and motd.5
bcr accepted this revision.Jun 24 2019, 9:49 AM
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
cem added a comment.Jun 24 2019, 4:01 PM

Thanks, Benedict!

delphij requested changes to this revision.Jun 25 2019, 4:37 PM
delphij added inline comments.
libexec/rc/rc.d/motd
34

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

TEMPLATE permission should be set here (or after line 34).

41

41-42: ln -sF "${TARGET}" "${COMPAT_MOTD}" would take care of the pre-existing file.

45

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–50

49-52 should be deleted. It's fine when ${TARGET} do not exist, we can generate its contents.

56–59

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

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.

libexec/rc/rc.d/motd
34

Thanks, will fix.

39

Will fix, thanks.

41

-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

Ok

49–50

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–59

Will fix.

usr.bin/login/motd.template
1

Will fix, thanks.

cem updated this revision to Diff 59939.Jul 19 2019, 7:12 PM
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.

libexec/rc/rc.d/motd
41

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–50

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.
libexec/rc/rc.d/motd
41

Ok, will change.

49–50

works for me!

cem updated this revision to Diff 59951.Jul 19 2019, 11:35 PM
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
delphij accepted this revision.Jul 20 2019, 1:43 AM
This revision is now accepted and ready to land.Jul 20 2019, 1:43 AM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.