Page MenuHomeFreeBSD

libprocstat: fix ZFS support
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.Thu, May 21, 2:03 PM
avg requested review of this revision.Thu, May 21, 2:03 PM
avg updated this revision to Diff 72074.Thu, May 21, 2:08 PM

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

markj added inline comments.Thu, May 21, 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.

avg added a comment.Thu, May 21, 4:02 PM

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.

avg updated this revision to Diff 72078.Thu, May 21, 4:04 PM

address markj's feedback

markj accepted this revision.Thu, May 21, 4:55 PM
This revision is now accepted and ready to land.Thu, May 21, 4:55 PM
kib added a comment.Thu, May 21, 5:07 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.Thu, May 21, 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.

This revision was automatically updated to reflect the committed changes.