[Toybox] Request to review a toybox patch (mdev)

[Rob Landley]

On 08/06/2018 10:15 AM, Eduardas Meile wrote:
> Hello,
> 
> I am a (now former) colleague of**Faustas Azuolas Bagdonas who has posted a
> patch to the toybox mailing list:
> 
> http://lists.landley.net/pipermail/toybox-landley.net/2018-July/009574.html

It was held up because it uses bool and the rest of the system doesn't, so I was
going to modify it not to. But since you pointed out people are using and
depending on this functionality, I just moved the header #include into mdev.c
and applied it otherwise as-is.

> Since no one has written any comments about it, I assume that it might have been
> just missed or the goal of the patch was unclear.

My todo list runneth over and $DAYJOB tends to eat my brain. (I meant to do this
yesterday but was so tired when I got home I never even opened my netbook. I did
it this morning before work because I knew somebody was waiting.) I see things,
but don't always immediately respond to them if I want to spend more time
reviewing or modifying something. (It goes on the todo list.)

In general, if I haven't responded to something in a week, ping me. Often the
delay means I want to do some minor modification to it that I can just as easily
do later.

> To clarify what the patch is
> for, I must tell you that me and Faustas wished to replace busybox with toybox
> for our dayjob Yocto-based embedded Linux distribution. We found all of the
> currently provided functionality in toybox 0.7.7 sufficient for that except for
> mdev.

Cool. What are you using for a shell? (That and route are the big blockers for
me in mkroot.)

> We need to set our block device names in our mdev.conf like this:
> 
> (sd[a-z])   root:disk 660 =usb_storage
> (sd[a-z])1  root:disk 660 =usb_storage_p1
> 
> This is how busybox does this, but mdev.conf of toybox only supports the first
> three fields for some reason. It is unclear to me whether this was a conscious
> decision. As far as I know there is no standard relating to this specific piece
> of toybox functionality,

There's no standard because I created it, and then it evolved based on user
feedback from the busybox community. The closest thing to a standard is
docs/mdev.txt in the busybox source.

Back in 2005 I noticed that the kernel experted a unique name for each device in
sysfs (the directory it was in) so you didn't _need_ lunacy like udev, you could
just populate /dev with that with a small shell script, the modern equivalent of
which would be something like:

for i in /sys/class/*/*/dev
do
  B=c
  [ "${i/block/}" != "$i" ] && B=b
  mknod "$B $(tr : ' ' <$i) /dev/$(echo "$i" | sed 's at .*/\([^/]*\)/dev@\1@')"
done

I used that script in my https://landley.net/aboriginal/old project but it took
~5 seconds to run so Frank Sorenson emailed me a C implementation of my script
that took half a second, and I went "you know, I could merge this into busybox"
and he gave me permission so I did.

http://lkml.iu.edu/hypermail/linux/kernel/0512.1/0595.html

The config file syntax was suggested by solar at gentoo on the old freenode #uclibc
irc channel. (There used to be logs of that but they went down.) At the time it
only had the three fields, and was based on his use cases. Other busybox
developers added the fourth field later.

I wrote up what I was thinking at the time:

http://lists.busybox.net/pipermail/busybox/2005-December/017180.html

And over the years n real-world complexity got piled on (as tends to happen).

> so perhaps you can recommend a more appropriate method
> of automatically setting device names with toybox mdev if this kind of patch is
> inappropriate. The reason we have to do this in the first place is related to
> our SWUpdate-based system update functionality.

My todo item for finishing this and moving it out of pending is basically "read
docs/mdev.txt and implement what it says", with the caveat that devtmpfs exists
now, which is "good enough" for 90% of embedded systems, and this probably
impact's mdev's design. (All mdev -s really has to do is trigger extra events,
the device nodes should already be there. I argued at the time that having
devtmpfs nodes have uid:gid values other than 0 was hardwiring policy in to the
kernel, but Greg did it anyway, so...)

Because of that I haven't actually used mdev myself in a while. I've also never
understood why people rename device nodes. (Create symlinks to them yes, rename
not so much. My proposal was "you can run a command line to do arbitrary things
when a hotplug event occurs", but they hardwired in = and > and does > _really_
have a symlink at the _original_ name rather than creating a symlink for the new
name? Move _and_symlink? Why would you...)

> If you require the patch to be modified for some reason before being accepted
> upstream, I would be glad to attempt that. Faustas no longer works at Fiber
> Optic Devices Ltd. (FOD) with me since last week, so he asked me to handle
> upstreaming of this functionality instead of him because of his lack of time.
> Currently the patch is being used in production for the Yocto-based builds that
> I do for our devices at FOD, so this functionality can be tested in a real
> industry usecase.

Cool. I'm happy to be guided by real-world users' needs.

> I would be very grateful if you were to take a look at the patch and let me and
> Faustas know if this kind of thing would be welcome upstream. If not, it would
> be good to know what we are doing wrong and what our alternative should be.
What I'd really like are regression test cases so when I get around to finishing
mdev I don't break it for existing users. Alas, mdev is one of this things that
needs a qemu instance to test, which is why I'm trying to get mkroot merged in,
which involves implementing toybox versionso fthe last 2 busybox commands it's
using. I can do "route" in a reasonable amount of time, but "sh" is a can of
worms...

Still, hotplug events are just a half-dozen environment variable values and
corresponding /sys file values. I could probably whip up something to harvest
them from real events and play them back... (See "my todo list runneth over",
above.)

> Best Regards,
> 
> Eduardas Meile

Rob