[Toybox] ls -laZ /dev takes a long time

[Rob Landley]

On 10/16/2015 02:56 PM, enh wrote:
> at the moment, ls.c has this:

Yeah, as the comment explains, darn awkward.

> but that doesn't work well in /dev.
> 
> root at shamu:/ # time ls -laZ /dev > /dev/null
>     0m10.33s real     0m0.05s user     0m0.62s system
> 
> some files, like /dev/smd_pkt_loopback, take a long time to fail to open:
> 
>      0.003022 openat(4, "smd_pkt_loopback",
> O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_NOATIME) = -1 EPROBE_DEFER (Unknown
> error 517)
>      5.185825 lgetxattr("/dev/smd_pkt_loopback", "security.selinux",
> "u:object_r:device:s0", 255) = 21

Which part of O_NONBLOCK is confusing that driver?

> it seems dangerous to be opening every file when running "ls", as
> opening a /dev file could have side effects or hang.

In theory O_PATH says I just want to access the metadata, not the
contents. In practice, whether xattrs are contents or metadata has never
been properly thought out (since the macos "resource fork" to store icon
positions in the finder was a hack people keep piling more work on top
of, the filesystem equivalent of TCP out-of-band data).

> if an open is
> really required, bionic's lgetxattr supports O_PATH file descriptors

Do you mean fgetxattr()? My ubuntu hasn't got the xattr function man
pages (no idea what package they're in) but:

http://linux.die.net/man/2/fgetxattr

Doesn't show a way to feed a filehandle in? (The open() man page says
fgetxattr() fails with such a filehandle, but the kernel could always
fix that or libc work around it.)

> as of https://android-review.googlesource.com/152663

Which is awesome, but if I'm going to support extended attributes it
would be nice to support them not _just_ on bionic.

Possibly the thing to do is an "if (CFG_TOYBOX_ANDROID) {do simple way}
else {do horrible thing};" but I'd sort of want to hide that in
lib/portability.c somehow.

(Sorry, I spent all my brain for the evening squinting at ps, after a
week of fighting with nommu toolchain issues. Plus I've hit the "massive
caffeine doses causing migraine symptoms" level that means time to detox
for a while, and that's zombie-making.)

> and the code for
> that's trivial, so we could just have toybox do that directly rather
> than whitelist bionic for O_PATH fgetxattr.

The code for what is trivial? (The above commit to libc touched a dozen
arch-specific files.) Do what directly instead of whitelisting bionic
for O_PATH fgetxattr? Wrap fgetxattr() in lib/portability.c?

I'm still sitting on something like 3 smack emails I need to get
through, and I'm trying to get ps to the point somebody can do a -Z
patch for it as fast as possible...

> happy to provide a patch if you indicate a preferred direction...

I'm not quite clear on what two alternatives you're proposing? I'm also
inclined to just trust your judgement here, because I'm not an xattr
expert, but I missed a curve and don't understand what my direction
options are here.

Thanks,

Rob

 1445055318.0