On Monday 23 June 2008 19:02, [EMAIL PROTECTED] 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 _______________________________________________ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox