Updated mdev

Rob Landley 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
> both
>
> 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 
kinda pointless.

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 
file.

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 
do this?

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 
place...)

So when this program is run with no arguments, it does nothing?  (Not even a 
help message?)

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.)

> Thanks,
>
> Frank

Rob
-- 
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 mailing list