mdev doesn't remove devices

Rob Landley rob at landley.net
Mon Aug 28 21:06:02 UTC 2006


On Wednesday 23 August 2006 4:54 am, Bernhard Fischer wrote:
> >+	if (!delete)
> >+	{
> >+		strcat(path, "/dev");
> 
> Didn't look, but would it be benefical to remove the constraint that
> path is pre-allocated and use concat_path_file() here?

Probably not.  Remember, we've got a recursive function in there.

I designed it with the pre-allocated path in mind.  It's not just polite to 
nommu systems, it saves a lot of work, is smaller in both code and runtime 
memory usage, gives much better cache locality (mostly in L1 cache, not even 
L2) and avoids thrashing the heap with repeated nested alloc/free pairs of 
varying sizes.

We're recursing down through the /sys directory, so we have to pass in 
the "path up to this point" each time.  Since that can't be larger than 
PATH_MAX, if we allocate one 4k buffer and track where it ends this time 
through the loop.  Then we can append the current path component to it at the 
end point (our starting point), and append the next one at the same place 
next time through the loop without having to free anything.  (This isn't a 
FEATURE_CLEAN_UP case because there could be an arbitrary number of devices 
in /sys, we would have to free it.)

If we can do that, then we can append the file component to it just as easily.  
It doesn't screw up our parent's memorized starting point, and we're not 
playing with any earlier part of the string.

I vaguely recall re-using some space at the end because I knew we'd have it.  
(PATH_MAX is 4k, and the deepest /sys path under class or block on my system 
isn't even 80 characters.  Of course greg keeps screwing things up so we have 
to care about /sys/devices, but it should still stay well under 4k.)

Note that if we allocate new non-overlapping strings at each recursion level, 
we can wind up allocating considerably _more_ memory than we otherwise would.  
(And thrash the heck out of the heap, which is slow.)

> Also, it looks like the initialization of *command above should be moved
> to below this "if (!delete)" clause -- potentially saves space.

Editing out some fluff, we have:

  char *command = NULL;

  if (ENABLE_FEATURE_MDEV_CONF) {command=blah;}

  if (command) ...

Now: if ENABLE_FEATURE_MDEV_CONF is 0, what will the compiler do?  (Answer: 
optimize out command entirely, via dead code elimination.)  If we move 
command=NULL into ENABLE_FEATURE_MDEV_CONF, we have an uninitialized variable 
path.

> Furthermore, why is umask(0) further down not placed into the "if
> (!delete)" clause as this seems to be the only place where it would be
> needed?

I don't think there's a umask(oldmask) anywhere in the program, so this could 
be anywhere.  It predates the shellout code (and for that matter hotplug 
support adding delete), but since shellout was implemented via system() and 
that can't affect the parent's umask, it might as well go to the start of 
main()...

> Did someone already look at mdev with size in mind? Just curious as i
> didn't look at it (yet)..

Yeah.  Me. :)

Still, a second pair of eyes is always welcome...

Rob
-- 
Never bet against the cheap plastic solution.



More information about the busybox mailing list