MODPROBE: next generation
Denys Vlasenko
vda.linux at googlemail.com
Tue Jun 24 16:00:35 UTC 2008
On Monday 23 June 2008 19:02, dronnikov at gmail.com wrote:
> Hello, Denys!
>
> Made a patch:
> 1. lsmod, modprobe, rmmod, depmod retain compatible interface: 1717 octets for all.
Wow.
> 2. insmod: do we really need to "publish" it as applet?
Ja ja. We love our users, we don't want to upset them by having
insmod disappear (or change interface).
> 3. modules.dep move to /tmp/modules.dep.bb: that way we ensure we can [re]create it.
I propose that an attempt should be made to copy it to
/lib/modules/$KERNELVERSION. If that fails, maybe issue a diagnostic
("cannot copy, will regenerate $FILE on every run [which is slow]").
Come to think about it, instead of "/tmp/modules.dep.bb" you may
call fd = mkstemp(DEFAULT_DEPMOD_FILE ".bb.XXXXXX") - makes you
safe versus modprobe races...
> Please, do try and test.
Testing.
modprobe with no parameters did generate a /tmp/modules.dep.bb,
which on the first glance seems to be ok. Strace:
execve("./busybox", ["./busybox", "modprobe"], [/* 32 vars */]) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
getuid32() = 0
chdir("/lib/modules") = 0
uname({sys="Linux", node="shadow", ...}) = 0
chdir("2.6.25-rc6") = 0
open("/tmp/modules.dep.bb", O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0644) = 3
...
But _depmod_ apparently failed to cd to $KERNELVERSION and as a result
it generated /tmp/modules.dep.bb for ALL kernels I have.
This is wrong. Strace:
execve("./busybox", ["./busybox", "depmod"], [/* 32 vars */]) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
getuid32() = 0
chdir("/lib/modules") = 0
uname({sys="Linux", node="shadow", ...}) = 0
unlink("/tmp/modules.dep.bb") = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0644) = 3
...
Will try to reboot with bb's modprobe_ng later. Now, comments on code,
some of them just nitpicking...
#include <libbb.h>
Most other applets use #include "libbb.h"
#undef CONFIG_DEFAULT_DEPMOD_FILE
#define CONFIG_DEFAULT_DEPMOD_FILE "/tmp/modules.dep.bb"
Shouldn't it be "/tmp/" DEFAULT_DEPMOD_FILE ".bb"?
static char* find_keyword(void *the_module, size_t len, const char * const word)
Well, const parameters buy you nothing, can use "const char *word" as well,
more readable (does not distract into "why is it * const? Is it something subtle?").
fd = (int)data;
...
fdprintf(fd, "%s ", name);
Well, can you use FILE-based io here? fdprintf is unbuffered, as a result,
modules.dep.bb generation is slow because of tons of small writes:
# cat modprobe.strace | wc -l
8128
# grep write modprobe.strace | wc -l
5124
static int inline already_loaded_issue(const char *name)
{
Don't use inline unless you must. gcc inlines single-callsite static funcs itself.
static int inline already_loaded_issue(const char *name)
{
// FIXME: overflow?
#define modules bb_common_bufsiz1
int ret = 0;
// N.B. can't use mmap or xmalloc_open_read_close()! They report wrong file length on mem-backed /proc/modules!!!
if (open_read_close("/proc/modules", modules, sizeof(modules)) >= 0) {
for (char *s = modules; (s = strstr(s, name)); s += strlen(name)) {
if (' ' == s[strlen(name)] && (s == s || '\n' == s[-1])) {
++ret;
break;
}
}
}
#undef modules
return ret;
}
* Can't you use local buffer? Or fix xmalloc_open_read_close to grow allocation
and continue reading if (unexpectedly) read() reads farther than stat.st_size.
* do strlen just once.
static int inline already_loaded_ugly(const char *name)
{
int ret = 0;
int fd = open("/proc/modules", O_RDONLY);
if (fd >= 0) {
#define modules bb_common_bufsiz1
// char *s;
while (reads(fd, modules, sizeof(modules))) {
if (' ' == modules[strlen(name)] && 0 == strncmp(modules, name, strlen(name))) {
++ret;
break;
}
}
#undef modules
}
return ret;
}
FILE-based io will look better here. reads() _seeks_ IIRC, that can be problematic
on /proc files.
// load it
extern int init_module(void *module, unsigned long len, const char *options);
size_t len = MAXINT(ssize_t);
Place externs at the top (below #includes).
char *options = strchrnul(ptr, ' '), *module_image;
Not readable. Is it "char *options = (strchrnul(ptr, ' '), *module_image);" ?
(Yes I know that it is not, but it looks confusing).
if (module_image)
free(module_image);
just free(module_image), free(NULL) is safe.
// lsmod called? ->
if ('l' == argv[0][0]) {
Will fail with "/bin/lsmod". Use applet_name[0].
// goto modules directory
if (option_mask32 & OPT_b)
xchdir(base_dir);
xchdir(CONFIG_DEFAULT_MODULES_DIR);
Will first xchdir be basically always ignored (CONFIG_DEFAULT_MODULES_DIR
usually starts with "/")?
int modprobe_main(int ATTRIBUTE_UNUSED argc, char **argv)
{
int ret = EXIT_FAILURE;
struct utsname uts;
char *modules_dep;
#define base_dir modules_dep
// lsmod called? ->
if ('l' == argv[0][0]) {
// just dump /proc/modules
bb_copyfd_eof(xopen("/proc/modules", O_RDONLY), STDOUT_FILENO);
goto done;
}
...
done:
if (ENABLE_FEATURE_CLEAN_UP)
free(modules_dep);
ret = EXIT_SUCCESS;
}
// TODO: more elaborate exitcode!
return ret;
}
free(modules_dep) would try to free garbage pointer.
--
vda
More information about the busybox
mailing list