Updated mdev

[Frank Sorenson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Rob Landley wrote:
> 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.

Good point, and you're right that I've got several of these.

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

Mostly because I seemed to be able to reimplement them smaller than if I
just used the functions.

"I'm an idiot" factors highly into this, I'm sure ;)

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

Not sure.  The udev code itself went this way, and I wasn't aware of
replacements.

> process_netlink_message:   Apparently, it can never get a block device 
> hotplugged over netlink, just char devices.

Both char and block devices should be handled.

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

I'm still an idiot :)

> Did you see any of the discussion on the busybox list about a minimal config 
> file format to specify ownership and permissions for devices?

Yes.  It shouldn't be too difficult to add the code to read the config
and set permissions and ownership.

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

I'm an idiot again?

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

Nice.  I figured it could be fixed for busybox by someone who knew what
they were doing.

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

Understood.  This was more a proof-of-concept and a first crack at
implementing it small.  It's functional code, but definitely not clean
and complete.

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

Please do.  It's working code, but obviously needs help.  Feel free to
take whatever you find helpful in your rewrite.

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

Frank
- --
Frank Sorenson - KD7TZK
Systems Manager, Computer Science Department
Brigham Young University
frank at tuxrocks.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFDnaLdaI0dwg4A47wRAka5AJ9a8uVa7NZwqn3Z4aoNQ0UtduVVsACgs7Y2
Wq/ZZvIK5XUg0B0lOny6jso=
=mX1F
-----END PGP SIGNATURE-----