[Pkg-sysvinit-devel] initscripts: please add code comment regarding corner-case mountpoint test (Re: lh_builds breaks without error message)

[Justin Pryzby]

Package: initscripts
Version: 2.86.ds1-38
Severity: wishlist

I previously sent to debian-live-devel at lists.alioth the following 

On Thu, Nov 22, 2007 at 08:54:56AM -0500, Justin Pryzby wrote:

> Is it ok to use initscript's /bin/mountpoint instead?  Erm, nvm, the logic in
> that tool seems to be broken (right?):
> 
> |       r = (st.st_dev != st2.st_dev) ||
> |           (st.st_dev == st2.st_dev && st.st_ino == st2.st_ino);
| |             printf("%s is %sa mountpoint\n", path, r ? "" : "not ");
| |        return r ? 0 : 1;

At first I thought that the test should be the usual-looking:

> |       r = (st.st_dev == st2.st_dev && st.st_ino == st2.st_ino);

However this is actually a test that it's the same file which isn't
what's wanted.

I think the first part of the test is right:

> |       r = (st.st_dev != st2.st_dev) ||

This says: "If a subdirectory is on a different device than its
parent, then some other device must be mounted there".  This is the
common reason why "mountpoint" would return true.

I decided that the 2nd part of the test is *also* right, although it
wasn't intuitive why:

> |           (st.st_dev == st2.st_dev && st.st_ino == st2.st_ino);

This says: "If a subdirectory *is the same node* as its own parent,
then (assuming this is a filesystem that uses inodes in a normal-ish
way or otherwise the same things hold true), then the same thing must
be mounted on the upper directory, *and* the corresponding filesystem
has a directory, the name of which is what's being tested for being a
mountpoint, *and* the same thing that's mounted on the upper directory
is also mounted on that directory".  This is a moderate subtlety of a
corner-case reason why mountpoint would return true.

Is this the right interpretation?

The initial code can effectively test (modulo compiler optimization)
the same equality twice; if (st.st_dev != st2.st_dev) is false, then
it's immediately tested again to see instead if the quantities are
equal (which is guaranteed).  This is one thing that caused me to
think that the code was wrong, since it wasn't(/isn't) clear to me
that this was understood at the time the code was introduced.

The test could actually be written

> |       r = (st.st_dev != st2.st_dev) || (st.st_ino == st2.st_ino);

However this looks just as wrong as the initial code (due to
similarity with the "is-the-same-file" test).  It's not clear to me
how to write this test in a self-documenting way, so I think this
deserves a code comment.