Thank you very much for constructive feedback. Most of the code is from nldev which I didn't write. I simply moved the child() function that fork/exec mdev to a separate helper.
I suppose I should rename it to something else than nldev. Maybe netlinkd? nlevd? The intention is that the helper should be an mdev modified to read events from stdin and timeout, instead of reading the event from environment. Alternatively, we could move the MODALIAS handling from mdev into the helper (using libkmod?) and only spawn mdev for non-MODALIAS events. That would actually reduce forks compared to now during coldplugging. A step forward. On Thu, 12 Mar 2015 17:51:00 +0100 Laurent Bercot <ska-dietl...@skarnet.org> wrote: > On 12/03/2015 16:19, Natanael Copa wrote: > > netlink listener code that needs to be in memory all the time: > > http://git.alpinelinux.org/cgit/ncopa/nldev/tree/nldev.c > > A few comments: > > - disableoom(): it's a generic operation for any daemon you don't > want to have killed, so you can factorize it: you can remove it from > the always-in-memory code, and run nldev under a program such as > perp's runchoom. http://b0llix.net/perp/site.cgi?page=runchoom.8 Good point. > - spawn_handler(): would benefit from being implemented via > posix_spawn(), to accommodate noMMU. On the other hand, you'd > need two elements in the posix_spawn_file_actions_t, so depending > on the implementation, that could pull in malloc(). It's a > trade-off. Interesting. I can use posix_spawn later if needed. > - sighandler()/initsignals(): trapping SIGKILL is a waste of code. of course :) > - init_netlink_socket(): listfd needs to be close-on-exec. yes. > - in sighandler() and at the end of main(): shutting down a socket > before closing it is a waste of code when you know there are no > copies of the socket. Shutting it down or closing it right before > exit()ing is also a waste of code. :) I suppose closing it is a good practice even if its a waste on linux. > - It may be worth it to write functions similar to die(), edie() > and dbg() that don't use stdio, and call _exit() instead of exit(). > If you manage to scrap stdio, you gain 2 pages of text (with the > musl implementation). Yeah. I think i should replace those funcs with something else. Good point that it should avoid stdio. I wonder if it would be worth to make dbg() a complie time thing so you need to run a special debug build for debugging. Would save a few bytes in the always-in-memory code. > - I understand that Alpine is runit-based. Save even more space > by scrapping all the dofork nonsense and saying that nldev should > be supervised. We use openrc, but this is still a good point. The daemonize stuff should be removed and handled by the caller. (or a supervising daemon ideally) > - line 213: ew. Please don't do defensive programming. Will remove. > - a pipe does not separate messages like a datagram socket does. > If you get a burst of events and nldev writes them all to the pipe, > nldev-handler won't be able to separate them. You need a message > terminator. In s6-uevent-listener, I simply used an extra \0 as > a message terminator. If you want to do the same: > * line 225: iov.iov_len = sizeof(buf)-1; > * insert after line 273: buf[len++] = 0; I think this is what I want to do yes. Will look into it. > - do you really mean to set the kernel buffer to 128 MB ? no. > > handler helper code: > > http://git.alpinelinux.org/cgit/ncopa/nldev/tree/nldev-handler.c > > More comments later ;) > Pushed fixes. Thanks! _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
