search

GuillaumeGomez / sysinfo

Cross-platform library to fetch system information
MIT License
2.08k stars 311 forks source link

Selectively refreshing disks #1375

Open dlowe opened 2 hours ago

dlowe commented 2 hours ago

When running Disks::new_with_refreshed_list() on android devices, I often see warnings (from SELinux?) along the lines of:

type=1400 audit(0.0:339351): avc:  denied  { search } for  name="block" dev="tmpfs" ino=12

I've done some spelunking, and it appears they are a side-effect of this canonicalize call, on certain disks' mount points.

(N.b. the dev="tmpfs" in the warning doesn't refer to the type of the filesystem being refreshed. I'm not using linux-tmpfs, so they're filtered out. In debugging, I've seen the warning for "ext4" and "fuse" filesystems.)

I know these warnings are harmless! ...but I've had customers who were alarmed about them, so I'd prefer to avoid generating them. Given my use-case (I'm only reporting disk usage over time for one disk), I'd love some sort of mechanism for selectively refreshing Disks. There are lots of options.

My first suggestion would be a separate Disks constructor (new_with_list() or new_with_unrefreshed_list()?) that loads the list of disks from /proc/mounts, but defers the stat collection until a later refresh. On linux, at least, I think the separation makes sense, and it gives complete control over what is refreshed to the library user, without adding much complexity to the interface. On other platforms, I haven't looked, but it seems like the worst-case would simply be that new_with_list() calls new_with_refreshed_list() on platforms where there's no distinction between listing and refreshing.

I'd be happy to do the work. If my proposal sounds reasonable, I'll submit a PR next week and we can hash out the specifics. If not, I'd be open to other ways of solving this... just let me know :)

GuillaumeGomez commented 1 hour ago

Seems a bit weird to want to have disks but not their path or even their kind. I think the best course would be to instead detect when the canonicalize call is actually needed and only call it if so.

dlowe commented 1 hour ago

Seems a bit weird to want to have disks but not their path or even their kind.

I was thinking those would be present in the new_with_list return. Only the information obtained from statvfs would be deferred.

I think the best course would be to instead detect when the canonicalize call is actually needed and only call it if so.

Hummm. Canonicalize (on linux) IIUC is just realpath. How would one decide if it were necessary? (I think you'd end up reimplementing almost all of realpath, right?)

GuillaumeGomez commented 1 hour ago

The kind is retrieved from the function using canonicalize, so both functions would get a call to it.

Hummm. Canonicalize (on linux) IIUC is just realpath. How would one decide if it were necessary? (I think you'd end up reimplementing almost all of realpath, right?)

Mostly checking if it's a link, if not, checking if it doesn't start with / or contains .. (still not great to check that ourselves).

Any clue why statvfs is emitting this warning?