Hmm. This is strikingly similar in purpose to the patch that I sent a couple
months ago ("mdev -s: don't mistake inter-device symlinks for separate devices";
using a different approach; it wasn't clear to me whether just disabling
symlink-traversal entirely was the right thing, but skipping the "device"
symlinks did seem to be right even if it was too narrow in scope).I guess I'm one of the "several people" you mentioned in your follow-up patch :) On 2015-08-03 17:17, Gregory Fong wrote: > On Mon, Aug 3, 2015 at 9:07 AM, Denys Vlasenko <[email protected]> > wrote: >> On Wed, Jul 15, 2015 at 11:10 PM, Gregory Fong <[email protected]> >> wrote: >>> From: Simon Edlund <[email protected]> >>> >>> When mdev -s traverses the /sys directory looking for "dev" files, it >>> starts with the block devices under /sys/block, and will find the "dev" >>> file through the symlink, and create a block device node. In the next >>> stage it will scan the /sys/class looking for char devices, and will >>> find mtdX/dev again, but this time the mknod will fail because there is >>> already a device node with that name. >> >> By swapping recursive_action("/sys/class") and >> recursive_action("/sys/block"), now the same thing will happen >> when the second scan is done. >> >> Why this is better? > > This is better because if you're using a newer kernel with both > CONFIG_SYSFS_DEPRECATED=y (and CONFIG_SYSFS_DEPRECATED_V2=y to enable > it), then where traversing /sys/block with the deprecated layout: > - /sys/block/mtdblock0 is not a symlink, but an actual directory > - /sys/block/mtdblock0/device is a symlink to mtd0 > > IOW, on deprecated sysfs: > # ls -l /sys/block/mtdblock0 > -r--r--r-- 1 root root 4096 Jan 1 00:00 alignment_offset > lrwxrwxrwx 1 root root 0 Jan 1 00:00 bdi -> > ../../devices/virtual/bdi/31:0 > -r--r--r-- 1 root root 4096 Jan 1 00:00 capability > -r--r--r-- 1 root root 4096 Jan 1 00:00 dev > lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> > ../../devices/platform/brcmnand.0/mtd/mtd0 > -r--r--r-- 1 root root 4096 Jan 1 00:00 discard_alignment > -r--r--r-- 1 root root 4096 Jan 1 00:00 ext_range > drwxr-xr-x 2 root root 0 Jan 1 00:00 holders > -r--r--r-- 1 root root 4096 Jan 1 00:00 inflight > drwxr-xr-x 2 root root 0 Jan 1 00:00 power > drwxr-xr-x 3 root root 0 Jan 1 00:00 queue > -r--r--r-- 1 root root 4096 Jan 1 00:00 range > -r--r--r-- 1 root root 4096 Jan 1 00:00 removable > -r--r--r-- 1 root root 4096 Jan 1 00:00 ro > -r--r--r-- 1 root root 4096 Jan 1 00:00 size > drwxr-xr-x 2 root root 0 Jan 1 00:00 slaves > -r--r--r-- 1 root root 4096 Jan 1 00:00 stat > lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem -> > ../../block > -rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent > > On non-deprecated sysfs: > # ls -l /sys/block/mtdblock0 > lrwxrwxrwx 1 root root 0 Jan 3 18:57 > /sys/block/mtdblock0 -> > ../devices/platform/rdb/f03e2800.nand/mtd/mtd0/mtdblock0 > > > > Now, I hadn't really investigated the problem carefully enough until > now. This patch simply solved a real problem for us when using > CONFIG_SYSFS_DEPRECATED, and I figured that submitting it would be at > least a good starting point for someone who actually understands this > better to figure out a better fix. > > But let's see... since recursive_action is called with > ACTION_FOLLOWLINKS, it will end up checking both > /sys/block/mtdblock0/dev and /sys/block/mtdblock0/device/dev and > creating block devices for both of those. That's not correct. So I > think I now have a real fix here, which is to change the > recursive_action call that will only be invoked if > CONFIG_SYSFS_DEPRECATED=y. In that case, we _shouldn't_ need to follow > symlinks because everything in the top level of /sys/block/ will be a > real directory, and following them will result in the aforementioned problem. > > It was a bit surprising to me that this problem isn't also seen with > /sys/class/block, since that has the same hierarchy where it could > follow the link and get to /sys/class/block/mtdblock0/device/dev, but > looking at recursive_action(), it appears that links are only followed > where depth == 0, which would explain that. > > I'm stuck using the gmail interface right now and so can't copy and > paste the new patch inline... will send to the list shortly. > > Best regards, > Gregory > >> >>> [gregory: added commit message to patch from BZ #6806] >>> Signed-off-by: Gregory Fong <[email protected]> >>> --- >>> util-linux/mdev.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/util-linux/mdev.c b/util-linux/mdev.c >>> index ca4b915..4495b0a 100644 >>> --- a/util-linux/mdev.c >>> +++ b/util-linux/mdev.c >>> @@ -1063,6 +1063,9 @@ int mdev_main(int argc UNUSED_PARAM, char **argv) >>> >>> putenv((char*)"ACTION=add"); >>> >>> + recursive_action("/sys/class", >>> + ACTION_RECURSE | ACTION_FOLLOWLINKS, >>> + fileAction, dirAction, temp, 0); >>> /* ACTION_FOLLOWLINKS is needed since in newer kernels >>> * /sys/block/loop* (for example) are symlinks to dirs, >>> * not real directories. >>> @@ -1079,9 +1082,6 @@ int mdev_main(int argc UNUSED_PARAM, char **argv) >>> ACTION_RECURSE | ACTION_FOLLOWLINKS | >>> ACTION_QUIET, >>> fileAction, dirAction, temp, 0); >>> } >>> - recursive_action("/sys/class", >>> - ACTION_RECURSE | ACTION_FOLLOWLINKS, >>> - fileAction, dirAction, temp, 0); >>> } else { >>> char *fw; >>> char *seq; -- "Don't be afraid to ask (λf.((λx.xx) (λr.f(rr))))." _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
