libprocstat: fix ZFS support
ClosedPublic
Actions

Authored by avg on May 21 2020, 2:03 PM.

Details

Reviewers

jhb
kib
markj
trasz

Commits

rS361363: libprocstat: fix ZFS support

Summary

First of all, znode_phys_t hasn't been used for storing file attributes for
a long time now. Modern ZFS versions use a System Attribute table with a
flexible layout. But more importantly all the required information is
available in znode_t itself.

It's not easy to include zfs_znode.h in userland without breaking code
because the most interesting parts of the header are kernel-only. And
hardcoding field offsets is too fragile. So, I created a new compilation
unit that includes zfs_znode.h using some mild kludges to get it and its
dependencies to compile in userland. The compilation unit exports
interesting field offsets and does not have any other code.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

avg created this revision.May 21 2020, 2:03 PM

Herald added subscribers: delphij, imp. · View Herald TranscriptMay 21 2020, 2:03 PM

avg requested review of this revision.May 21 2020, 2:03 PM

Harbormaster completed remote builds in B31214: Diff 72073.May 21 2020, 2:03 PM

catch up with the fact that zfs.c is a "normal" file now

Harbormaster completed remote builds in B31215: Diff 72074.May 21 2020, 2:08 PM

markj added inline comments.May 21 2020, 3:59 PM

lib/libprocstat/zfs.c
32 ↗	(On Diff #72074)	From style(9), "<sys/param.h> includes <sys/types.h>; do not include both."
79 ↗	(On Diff #72074)	We leak dataptr here.
83 ↗	(On Diff #72074)	Doesn't it make sense for this check to come right after the sysctl fetch?
lib/libprocstat/zfs_defs.c
27 ↗	(On Diff #72074)	Duplicate RCS ID.

Thank you for the feedback.
Working on it.

lib/libprocstat/zfs.c
83 ↗	(On Diff #72074)	It does, but I wanted to avoid printing both this message and "too small" message in the case where the size is indeed too small. So, I moved this check here.

address markj's feedback

Harbormaster completed remote builds in B31216: Diff 72078.May 21 2020, 4:04 PM

markj accepted this revision.May 21 2020, 4:55 PM

This revision is now accepted and ready to land.May 21 2020, 4:55 PM

I think my notes are mostly not about your patch, but rather reflect my serious dislike of the original rude implementation.

lib/libprocstat/Makefile
61 ↗	(On Diff #72078)	Why this needs to go into subdir ?
lib/libprocstat/zfs.c
33 ↗	(On Diff #72078)	What bits do you need from sys/mount.h ? Look for an example of sys/file.h with _WANT_FILE. #define _KERNEL is a way to cause innocent sys commits to break world.

avg added inline comments.May 21 2020, 8:35 PM

lib/libprocstat/Makefile
61 ↗	(On Diff #72078)	I don't have a good answer, but my guess it's so that the separate Makefile is used -- with different compiler flags, include paths, etc.
lib/libprocstat/zfs.c
33 ↗	(On Diff #72078)	This is for `mount.mnt_stat.f_fsid.val[0]` that's used to get filesystem ID. Not sure if that's the right way, the code was there. Also, I see that smbfs "plugin" also uses the same approach.