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 25436
Build 24064: 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
29–30

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

33

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}"

38

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

40

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

44

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

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
0–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
29–30

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.

33

Thanks, will fix.

38

Will fix, thanks.

40

-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.

44

Ok

49–52

Will fix.

usr.bin/login/motd.template
0–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
29–30

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.

40

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.

cem marked an inline comment as done.Jul 19 2019, 11:33 PM
cem added inline comments.
libexec/rc/rc.d/motd
29–30

works for me!

40

Ok, will change.

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.