Page MenuHomeFreeBSD

Remove dependency of rc.d/tmp on grep
ClosedPublic

Authored by se on Jan 17 2021, 12:29 PM.

Details

Summary

The committed version of /etc/rc.d/tmp depends on "mountcritremote" due to its use of grep.

This dependency can easily be removed to allow rc.d/tmp to be executed before /usr/bin is mounted.

Use of a here document avoids complications due to the spawned sub-shell if "df /tmp | while ..." was used.
In that case, the sub-shell would return a success code which then needed to be tested.

If a version without here-document was found to be preferential, I'd suggest a shell function that returns the result of "df /tmp | grep ..." but which does use the while loop from this patch instead of grep.

Test Plan

Apply patch and verify that "mount_md" is invoked if there is no line matching "^/dev/md[0-9].* /tmp" in the output of "df /tmp".

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

se requested review of this revision.Jan 17 2021, 12:29 PM
se created this revision.

This fixes at least one dependance on /usr, I see another later, mktemp which is /usr/bin/mktemp. I would of marked it above, but a full file context was not uploaded to this review.

This revision is now accepted and ready to land.Jan 17 2021, 2:09 PM

Remove dependency on mktemp, too, since it also resides in /usr/bin, as noted by rgrimes.

This revision now requires review to proceed.Jan 17 2021, 2:44 PM
cy requested changes to this revision.Jan 17 2021, 2:46 PM

Otherwise it looks good. This has the advantage of:

  1. Less dependency on /usr, as rgrimes mentioned on src-committers@.
  2. Less fork()ing.
libexec/rc/rc.d/tmp
50

Another way to do this, might be df /tmp | while read line... To me it looks cleaner.

This revision now requires changes to proceed.Jan 17 2021, 2:46 PM

As written in the comment of the original submission, a version based on "df /tmp | while ..." can be used instead of the here-doc.
But after implementing both, I did consider the here-doc based version to be both easier to understand and more efficient and thus only proposed that version.

libexec/rc/rc.d/tmp
50

My initial attempt actually used "df /tmp | while ...", but I find the here doc version more straight forward.

Here is a version that uses "df /tmp | while ...":

mount_tmpmfs()
{
        df /tmp | while read line; do
                case $line in
                /dev/md[0-9]*\ /tmp)
                        return 1;;
                esac
        done
        if [ $? = 0 ]; then
            mount_md ${tmpsize} /tmp "${tmpmfs_flags}"
            chmod 01777 /tmp
        fi
}

The while loop is executed in a sub-shell and thus needs to be exited with "return 1" to pass a value to the if condition.

This is easy to understand if you know about the while loop being executed in a sub-shell, I think that the here-doc version does not require such knowledge.

The pipe-based version requires 2 forked sub-processes (the df command and the shell executing the while loop), but the here-doc version only forks the df process and I do prefer it for these reasons.

Stefan, thanks a lot of taking care of my fallout!

libexec/rc/rc.d/tmp
50

To me the here-doc looks more efficient than pipe.

I had tried to create the temporary directory name without dependency on /dev/random (and thus suggested to use "ps | sha256", or in my temporary version "ps lauxww | grep sha256").

But since mktemp uses random(4), the currently committed version did also depend on /dev/random being operational.
Therefore, the version I want to commit uses "dd if=/dev/random" to generate the test directory.
(If sufficient entropy was not available, "ps lauxww" might actually be preferential, but then I'd expect that there must have been reports of mktemp blocking ...)

Agree with glebius.

This revision is now accepted and ready to land.Jan 18 2021, 9:12 PM
This revision was automatically updated to reflect the committed changes.