rob at landley.net
Mon Dec 12 07:57:58 UTC 2005
On Sunday 11 December 2005 23:23, Frank Sorenson wrote:
> Attached is an updated mdev. Changes from the previous version:
> - bugfixes mentioned on this list
> - mdev now dynamically adds and removes device nodes
> - command line arguments to process /sys once, monitor for changes, or
> mdev compiles to < 7K
> Comments are welcome.
Highly cool. Let's take a look...
First thing I noticed paging through:
A) file_exists() is only used in one place, so making a function for it is
B) the switch statement is unnecessary complication; if stat() returns -1 for
root (which this has to be running as), you can assume we can't access this
C) "man 2 access". The easy way to ask "does it exist" is access(file,0);
Looking more closely, you also seem to have reimplemented strcat
(string_cat()), strncmp (string_cmpn()), memset (clear_mem())... Why did you
Looking at get_device_info (which is another function that's called from only
one place): BUFFER_LENGTH was chosen how? (I'd be more comfortable with
bb_asprintf() here.) The check if (ret ==0 ) leaks the filehandle, and
doesn't catch the actual error return value (-1) which would leading to the
buf[ret-1]=0 assignment writing outside the buffer...
All this stuff with netlink needs a CONFIG_ and #ifdefs. Is there _really_ no
better header to include than linux/netlink.h and linux/types.h?
process_netlink_message: Apparently, it can never get a block device
hotplugged over netlink, just char devices. Why bother to check if the file
exists before calling unlink? Why do you care about unknown messages from
netlink enough to print a message (and filter out a known message about
modules which you don't intend to do anything about, just to avoid printing
this message)? Why does this function bother returning anything if it's
always going to be 0?
Why is usage() a function? Or run_daemon()? (Again, called from only one
So when this program is run with no arguments, it does nothing? (Not even a
Did you see any of the discussion on the busybox list about a minimal config
file format to specify ownership and permissions for devices?
What's the deal with local_errno? (You're not using threads, so why cache the
value in a local variable?) Errors should go to stderr (dprintf(2,blah) or
fprintf(stderr,blah)). (By the way, in busybox we have our own library
shared between applets, and that has error printing functions anyway. But I
don't expect you to know that...)
If nobody ever pays attention to the return value of make_device(), why does
it bother to return different values? Similarly, the one and only caller of
get_device_info() wouldn't have to deal with marshalling arguments back
through integer pointers if the code was just inline.
On the whole it's cool, but not something I'd merge right now.
I have plans to tackle mdev for busybox at the start of 1.2. (Including the
config file functionality and adding it to applets.h and usage.h and kconfig
all that.) I see several chunks of code I can use in here, but I'd clean it
up noticeably. (Heck, I might take a stab at it tomorrow if I get sick of
bug-whacking. Currently going through my triage.txt file and closing out
bugs from bugs.busybox.net.)
Steve Ballmer: Innovation! Inigo Montoya: You keep using that word.
I do not think it means what you think it means.
More information about the busybox