frank at tuxrocks.com
Mon Dec 12 16:18:37 UTC 2005
-----BEGIN PGP SIGNED MESSAGE-----
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
> 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
> 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
> 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
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
> 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 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
-----END PGP SIGNATURE-----
More information about the busybox