[BusyBox] [patch] new applet mountpoint

[Rainer Weikusat]

Bernhard Fischer <rep.nop at aon.at> writes:
> On Fri, Aug 19, 2005 at 04:30:02PM +0200, Rainer Weikusat wrote:

> [...]

> >>Well, there may be reasons why someone would want to not have tail,
> >>but that's not the same question as "Why mountpoint.c", especially
> >>considering that your implementation is both broken due to various
> >>races (the stat/ lstat calls, for instance), needlessly
> >
> > Hm. the sequence is stat /dir && stat /dir/.. || exit failure;
> > Please explain that race?

The race would happen between the applet and some other process (or
set of processes) that work with the filesystem (deleting, renaming or
creating directories, for instance). You cannot assume that a path
name which you used as input for one system call still refers to the
same filesystem object in a subsequent system call. The usual practice
to deal with this would be to open the directory and then use fchdir/
fstat. A reasonably looking suggestion would be:

 	chdir(path);	/* prevent deletion/ umounting */
        fd = open(".", O_RDONLY, 0); /* avoid parsing on futher operations */

and then use fstat on the descriptor.        

>>inefficient (pointless memory allocations) and does some stuff which
>
> If you refer to the call to bb_xasprintf, perusing this was smaller than
> doing it by hand.

Just because the code is somewhere else does not mean it is no longer
there.

> Care to explain how you would accomplish this more
> efficiently?

Given that my cwd would be the directory I am interested in (cf
above), I would use ".." ;-). 

> What other inefficiencies (apart from the strerror) do you mean? Just
> curious.

One example:

			if (stat(p, &st) == 0) {
				short ret = (st_dev != st.st_dev) ||
					(st_dev == st.st_dev && st_ino == st.st_ino);
				if (opt & OPT_d)
					bb_printf("%u:%u\n", major(st_dev), minor(st_dev));
				else if (!(opt & OPT_q))
					bb_printf("%s is %sa mountpoint\n", arg, ret?"":"not ");
				return !ret;
			}

This could be reformulated to be somewhat less jumpy (which is bad for
pipelined processors) and to use machine words where feasible (good
for RISC processors):

char const *strings[] = { "", "not " };

if (stat(p, &st) == 0) {
    int no_mountpoint =
        (st_dev == st.st_dev) & (st_ino != st.st_ino);

    opt &= OPT_d | OPT_q;

    if (opt) {
        if (opt > OPT_q) bb_printf("%u:%u\n", ...);
    } else
        bb_printf("%s is %sa mountpoint\n", strings[no_mountpoint]);            
                 
    return no_mountpoint;
}                
            
This would of course need to be tested, but could be something worth
investigating if performance was of concern. In any case, it is much
easier to read (less text), though not necessarily easier to
understand :-) (requires basic knowledge of boolean algebra and the
ability to actually use it).

>>is hardly useful (like printing a single \n with printf
>>(putchar, anyone?) when told to be quiet) and does some things which
>
> You're right on the putchar. I agree that it looks a bit odd, but
> sysvinit does print that newline so i added it to be compatible.

The one in Debian/ sarge doesn't and I would consider this a bug,
too. 

>>are just weird (why the 'stat/lstat'-combination)?
>
> My copy of the mountpoint manpage reads:
> [...]
> Symbolic  links are not followed, except when the -x option is used.
> [...]

That would be another bug in Miguel's code :-> (I would agree that a
directory with two parents is somewhat of an abomination, but
inconsistency is one, too. So either follow the link or don't and
don't follow the pretty link and not the ugly one).