Re: MODPROBE: next generation
[CC: [EMAIL PROTECTED] On Sunday 06 July 2008 08:27, Vladimir Dronnikov wrote: Hello, Denys! Looked through modutils-small.c. Could you explain the changes you made? I see the code became 500 bytes larger... I made the following changes: * directory scannig is done just once * if we find module to modprobe during dirscan, we immediately try to load it. If it succeeds, we exit. This makes modprobe MUCH faster in some quite coomon cases. * otherwise, we remember all module pathnames, so that we do not need to scan directory again. * If module was found but failed to load, we read ONLY that module's body, determine deps, scan in-memory list of modules, and try to load deps (recursively doing the same read body, find deps for each of them). * ONLY if module was not found by name, we read ALL module bodies and find alias names. Very time consuming. Still, as soon as alias is found, the module body reading stops (cuts time down by 50% on average). Then we try to load found module as above. I decided to get the above logic working correctly so that modprobe wouldn't scan whole module directory, and read each and every module in it on every call; and turn my attention to shrinking it later. -- vda ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
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.XX) - 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, '
Re: MODPROBE: next generation
On Tuesday 24 June 2008 18:00, Denys Vlasenko wrote: 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.XX) - makes you safe versus modprobe races... Please, do try and test. Another thing I want to ask: please document modules.dep.bb format in a big comment at the top of modprobe-ng.c. Be verbose there. -- vda ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Tuesday 24 June 2008 23:48, [EMAIL PROTECTED] wrote:
Hello, Denys!
Made a new patch. Tried to fix the issues you pointed out.
+ do {
+ /* search for the first char in word */
+ ptr = memchr(ptr, *word, len - (ptr - (char*)the_module));
+ if (ptr == NULL) /* no occurance left, done */
+ return NULL;
+ if (!strncmp(ptr, word, strlen(word))) {
+ ptr += strlen(word);
+ break;
+ }
+ ++ptr;
+ } while (1);
Can you move strlen outside of the loop?
+ fprintf(fp, \n);
Nitpicking department says fputc.
+static void process_module(char *modules_dep, char *name
+ USE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE(, const char
*cmdline_options))
+{
...
+ process_module(modules_dep, s
+
USE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE(, cmdline_options)
+ );
There is a neat trick how to make it readable. Put this before
the definition:
#if !ENABLE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE
#define process_module(a, b, c) process_module(a, b)
#endif
+ // if depmod called -
+ if ('d' == applet_name[0]) {
+ // force recreating modules.dep.bb
+ // N.B. to cope well-known sendmail bug we should ensure
+ // we are not writing to a symlink (to, e.g., /etc/passwd)
+ // placed by a malicious user in world-writable /tmp directory
+ // OTOH if we just unlink(DEPMOD_FILE) our depmod can kill smth
unwillingly.
+ // So I prefer to bail out if depmod has to overwrite smth.
Comments?
+ struct stat st;
+ if (!lstat(DEPMOD_FILE, st)) {
+ if (!(option_mask32 OPT_q)) {
+ // complain
+ errno = EEXIST;
+ bb_perror_msg_and_die(DEPMOD_FILE);
+ } else
+ goto quit;
+ }
+ }
I think depmod must create modules.dep.bb in /lib/modules, not in /tmp.
Just bail out if it can't create a file there.
+ if (!modules_dep) {
+ // depmod: dump modules definitions
+ // TODO: locking!!!
+ // TODO: locking!!!
+ // TODO: locking!!!
+ // TODO: locking!!!
+ // TODO: locking!!!
+ FILE *fp = fopen(DEPMOD_FILE, w);
+ if (fp recursive_action(.,
+ ACTION_RECURSE, /* flags */
+ fileAction, /* file action */
+ NULL, /* dir action */
+ fp, /* user data */
+ 0) /* depth */
+ ) {
+ fprintf(fp, \n); // finalize definitions
+ fclose(fp);
+ // depmod was called? - we're done
+ if ('d' == applet_name[0])
+ return EXIT_SUCCESS;
+ // read definitions
+ modules_dep = xmalloc_xopen_read_close(DEPMOD_FILE,
NULL);
+ }
+ }
Locking can be provided by atomic rename. Roughly:
// we are in /lib/modules[/X.Y.Z]
template = xstrdup(DEPMOD_FILE XX);
fd = mkstemp(template);
fp = fdopen(fd, w);
generate_modprobe_dep_bb(fp);
fclose(fp); // closes fd too
rename(template, DEPMOD_FILE); === atomically replaces modules.dep.bb
Please, try it.
Will do in a minute.
--
vda
___
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Mon, Jun 23, 2008 at 10:02:51AM -0700, [EMAIL PROTECTED] wrote: Hello, Denys! Made a patch: 1. lsmod, modprobe, rmmod, depmod retain compatible interface: 1717 octets for all. 2. insmod: do we really need to publish it as applet? 3. modules.dep move to /tmp/modules.dep.bb: that way we ensure we can [re]create it. Please provide bloat-o-meter output. cheers, ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Tuesday 17 June 2008 19:24, [EMAIL PROTECTED] wrote: 0. Intro depmod + modprobe is some kind of complicated codec. Though the syntax of modprobe.conf and friends is human-friendly most of module dependencies is intrinsic and are hardly a subject to be customized manually. du modules.* gives 150Kb on my reasonably small system and this amount of loosely defined textual data must be parsed at every modprobe invocation. I suggest to invent for BB a custom modules.dep format which bites all above issues. 1. Format of modules.dep (draft) Consider a module X which depends on Y and Z, has aliases X1 and X2 and must be loaded with options O1 and O2. We could write this definition down as follows: --- X Y Z X1 X2 O1 O2 --- If any of parts of definition is missed we use empty line for it. No more continuation lines, delimiters and so on! Just mmap() the whole file and count for well-known amount of EOLs for each module and use skip_whitespace(). offI wish we had gzmmap() invented (or have we?)/off Extensions to such a format can be done trivially. 9. Miscellany depmod can be trivially (IMO) updated to generate new modules.dep Personally I think of depmod as of something redundant in terms of BB. modules.dep could be generated on-the-fly during the first call to modprobe if it is missed. To not interfere with old-style modutils new modules.dep should be named, say, modules.bb. Comments? I must agree with others who say that divergence from standard format is undesirable. I see that your main concern is the size of this file (modprobe's execution time is not so important since you do not run it often). How about just teaching modprobe to use modules.dep.gz is it exists, with fallback to modules.dep? We already have open_transformer(), so you just call fd = open_transformer(gzfile_fd, unpack_gz_stream, gunzip); and voila - you can gunzip file on the fly :) gzip compresses modules.dep formatted file to ~1/20th of its size: # wc -c modules.dep 60646 # gzip modules.dep | wc -c 3876 What do you think? -- vda ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Fri, Jun 20, 2008 at 04:58:04PM +0200, Denys Vlasenko wrote: We already have open_transformer(), so you just call fd = open_transformer(gzfile_fd, unpack_gz_stream, gunzip); and voila - you can gunzip file on the fly :) Can we have a libunarchival wrapper that sets up open_transformer for us, depending on supported decompressors and file-extension (see depmod.c for the FIXME) ? tar already has such a thing, ISTR.. perhaps generalize that ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Friday 20 June 2008 17:05, Bernhard Fischer wrote: On Fri, Jun 20, 2008 at 04:58:04PM +0200, Denys Vlasenko wrote: We already have open_transformer(), so you just call fd = open_transformer(gzfile_fd, unpack_gz_stream, gunzip); and voila - you can gunzip file on the fly :) Can we have a libunarchival wrapper that sets up open_transformer for us, depending on supported decompressors and file-extension (see depmod.c for the FIXME) ? tar already has such a thing, ISTR.. perhaps generalize that tar looks at first few bytes, not at the extension. If only kernel had ungetc for fd-based io... I think it is somewhat unfair to ask Vlad to do this, we can add it incrementally later. -- vda ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
Re: MODPROBE: next generation
On Tue, 2008-06-17 at 10:24 -0700, [EMAIL PROTECTED] wrote: I suggest to invent for BB a custom modules.dep format which bites all above issues. ... Comments? Please no. My alpine linux distro is based on uclibc/busybox. If hte busybox utility is not good enough for user, then it is possible to install the real utility that just overrides the busybox version. This requires that they behave similar. Otherwise scripts and things tend to break. (so far its only the sendmail applet that causes problems so i had to turn it off) So, please, continue make busybox compatible with POSIX/GNU standards. -nc ___ busybox mailing list busybox@busybox.net http://busybox.net/cgi-bin/mailman/listinfo/busybox
