Add the ability to detect ZFS and GELI encrypted file systems to fstyp(8)

[allanjude]

Summary

Please check the code carefully, as I am a very novice C programmer
The Makefile requires especially careful review, I have no idea what I am doing, I
just kept checking things until it worked.

[imp]

usr.sbin/fstyp/fstyp.c

65

so what makes zfs mountable, and ufs not mountable?

[allanjude]

usr.sbin/fstyp/fstyp.c

65

Other way around. When a new device is attached, it will be auto-mounted, unless "unmountable" is true (for ZFS and GELI), since they can't be mounted with just 'mount -t <type> <device> <mountpoint>'. ZFS requires 'zpool import' and GELI needs a passphrase.

So fstyp -u <device> can detect these types, but when run without -u (by autofs etc), zfs and geli will be detected as unknown, and skipped.

[eadler]

usr.sbin/fstyp/Makefile

29

LIBADD= geom md ...
this eliminates need for DPADD and LDADD I think

31

combine with CFLAGS above

usr.sbin/fstyp/fstyp.c

145

this is technically against style(9) but I actually prefer this style.

flags should be named "uflag", "iflag", etc. and initialized after declarations are finished.

the way it is above is far better though

usr.sbin/fstyp/geli.c

47

double checking tabbing

usr.sbin/fstyp/zfs.c

64

check return value?

[trasz]

So, apart from the missing MK_CDDL or MK_OPENSOLARIS, it looks pretty much done?

Oh - the manual page!

usr.sbin/fstyp/zfs.c

69

I think so; in short running processes, like fstyp, it doesn't really matter that much.

[allanjude]

Makefile fixes

[allanjude]

Still need to do manpage

usr.sbin/fstyp/Makefile

29

LIBADD doesn't seem to support nvpair or zfs yet. They are not defined in share/mk/src.libnames.mk and produce an error.

[allanjude]

Update man page and usage statement

[trasz]

usr.sbin/fstyp/Makefile

29

I'd say the correct course of action then is to add those to src.libnames.mk, and use LIBADD. Or at least investigate why this hadn't been done - just an overlook, or some larger problem (ABI?).

usr.sbin/fstyp/fstyp.8

99

How about "Include filesystems such as ZFS and GELI that cannot be mounted directly using mount(8)"?

[eadler]

docs only reviewed; please fix the nit I pointed out

usr.sbin/fstyp/fstyp.8

128

please fix this

[allanjude]

Feedback from trasz

[allanjude]

usr.sbin/fstyp/Makefile

29

This is well outside of my level of understanding of the build system. Do you know who I should ask for assistance?

[trasz]

usr.sbin/fstyp/Makefile

29

bapt@, I believe.

[allanjude]

usr.sbin/fstyp/Makefile

29

<bapt> AllanJude: nothing cddl related has been converted to libadd
<bapt> and all libs there are a giant mess
<bapt> not easy to do
<bapt> for example libzfs_core depends on libzfs which depends on libzfs_core

So the suggestion seems to be to leave it as DPADD/LDADD for now.

[wblock]

usr.sbin/fstyp/fstyp.8

46–57

ZFS should probably not be mentioned here.
Add a sentence that says the normal output is meant to be used by an automounter or .Xr mount 8.

47

This sentence is not needed (see below).

50

The parts about ZFS conflict between these two sentences, and the second one is kind of confusing. Better to tell the user why:

When the
.Fl u
flag is specified,
.Nm
acts as a general-purpose filesystem and also identifies ZFS filesystems and
.Xr geli 8
devices.

51

This sentence is not needed.

99

GELI is not a filesystem, though. Might be better to be less specific:

Include filesystems and devices that cannot be mounted directly by

113

geli comes before glabel (sort by alpha within the category number).

126

"support" is singular, so s/were/was/.

[allanjude]

usr.sbin/fstyp/fstyp.8

126

That reads oddly to me. How is: "Support for ZFS and GELI were added by"

[allanjude]

man page updated with feedback from wblock@

[trasz]

usr.sbin/fstyp/fstyp.8

51

I agree - I'd drop the "The output of ... manually with mount(8)" sentence, and add something like wblock@ suggested - "When the -u flag is specified, fstyp also recognizes certain metadata formats that cannot be handled using mount(8), eg. ZFS pools and GELI providers".

99

Remove "special"?

[allanjude]

Feedback from trasz

[wblock]

usr.sbin/fstyp/fstyp.8

126

"Support" is still singular, so "were" doesn't work: "Support ... were added by".

But it can be rearranged:
"ZFS and GELI support was added by"

[allanjude]

After getting a second oppinion, I've been convinced I was wrong, and 'were' is not correct.

[emaste]

usr.sbin/fstyp/fstyp.8

126

Or even "Allan Jude added ZFS and GELI support."

[trasz]

Allan, committing this will also close https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=200823; could you take that ticket?

[asomers]

usr.sbin/fstyp/fstyp.8

56

s/provides/providers

[allanjude]

Updated with feedback from asomers

[allanjude]

Is this ready for commit now?

[wblock]

Man page looks okay. Please test with igor and mandoc -Tlint also. Thanks!

[allanjude]

Change to using MK_ZFS instead of MK_CDDL to avoid issues when user sets WITHOUT_ZFS

Change WARNS level

[trasz]

Erm, what's the problem with WARNS?

[trasz]

Also, could you take https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=200976?

[allanjude]

In D2045#55636, @trasz wrote:

Erm, what's the problem with WARNS?

GCC

via kib on irc:

> usr.sbin/fstyp (all)

cc1: warnings being treated as errors
In file included from
/scratch/tmp/kib/src/usr.sbin/fstyp/../../sys/cddl/contrib

from /scratch/tmp/kib/src/usr.sbin/fstyp/zfs.c:41:

n/fs/zfs/sys/dmu.h:638: warning: function declaration isn't a prototype
n/fs/zfs/sys/dmu.h:639: warning: function declaration isn't a prototype

So it had broken the build on all platforms that still use GCC

[allanjude]

In D2045#55644, @trasz wrote:

Also, could you take https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=200976?

Yes, this person contacted me on IRC, I wasn't aware they also filed a PR.

My proposed change should solve this, but I am still testing it via make universe

[trasz]

I'd rather fix the problem instead of papering it over by decrementing WARNS. WARNS is nice. I like it.

That GCC message - that two routines, could you perhaps try to fix their arguments (basically add 'void') and see if that fixes the build, with WARNS=6?

[allanjude]

In D2045#55691, @trasz wrote:

I'd rather fix the problem instead of papering it over by decrementing WARNS. WARNS is nice. I like it.

That GCC message - that two routines, could you perhaps try to fix their arguments (basically add 'void') and see if that fixes the build, with WARNS=6?

With some style-ish patching to ZFS, I can get WARNS=5 to work, but WARNS=6 still has issues

In file included from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/compat/opensolaris/sys/debug.h:39,

from /media/svn/fstyp/head/usr.sbin/fstyp/../../cddl/contrib/opensolaris/lib/libzpool/common/sys/zfs_context.h:69,
from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/spa.h:31,
from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/metaslab.h:29,
from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/vdev_impl.h:31,
from /media/svn/fstyp/head/usr.sbin/fstyp/zfs.c:41:

/media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/compat/opensolaris/sys/assfail.h:65: warning: redundant redeclaration of 'aok'
/media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/compat/opensolaris/sys/assfail.h:49: warning: previous declaration of 'aok' was here

In file included from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/spa.h:31,

from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/metaslab.h:29,
from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/vdev_impl.h:31,
from /media/svn/fstyp/head/usr.sbin/fstyp/zfs.c:41:

/media/svn/fstyp/head/usr.sbin/fstyp/../../cddl/contrib/opensolaris/lib/libzpool/common/sys/zfs_context.h:134: warning: redundant redeclaration of 'aok'
/media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/compat/opensolaris/sys/assfail.h:65: warning: previous declaration of 'aok' was here

In file included from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/spa.h:31,

from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/metaslab.h:29,
from /media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/vdev_impl.h:31,
from /media/svn/fstyp/head/usr.sbin/fstyp/zfs.c:41:

/media/svn/fstyp/head/usr.sbin/fstyp/../../cddl/contrib/opensolaris/lib/libzpool/common/sys/zfs_context.h:563: warning: redundant redeclaration of 'highbit64'
/media/svn/fstyp/head/usr.sbin/fstyp/../../sys/cddl/contrib/opensolaris/uts/common/sys/sysmacros.h:425: warning: previous definition of 'highbit64' was here

[trasz]

Hm, could you try something like this:

Index: sys/cddl/compat/opensolaris/sys/assfail.h

• sys/cddl/compat/opensolaris/sys/assfail.h (revision 284597)

+++ sys/cddl/compat/opensolaris/sys/assfail.h (working copy)
@@ -62,7 +62,9 @@
#endif

#ifndef HAVE_ASSFAIL3
+#ifdef HAVE_ASSFAIL
extern int aok;
+#endif

static inline void
assfail3(const char *expr, uintmax_t lv, const char *op, uintmax_t rv,

[allanjude]

In D2045#55719, @trasz wrote:

Hm, could you try something like this:

Index: sys/cddl/compat/opensolaris/sys/assfail.h

• sys/cddl/compat/opensolaris/sys/assfail.h (revision 284597)

+++ sys/cddl/compat/opensolaris/sys/assfail.h (working copy)
@@ -62,7 +62,9 @@
#endif

#ifndef HAVE_ASSFAIL3
+#ifdef HAVE_ASSFAIL
extern int aok;
+#endif

static inline void
assfail3(const char *expr, uintmax_t lv, const char *op, uintmax_t rv,

I was thinking that it would be cleaner, if above the two sets of #ifndef's we just did:
#if defined(HAVE_ASSFAIL) || defined(HAVE_ASSFAIL3)
extern int aok;
#endif

to do it once, for any of the 3 cases where it is required, and then do the rest conditional as before.

[dan.mcgregor_usask.ca]

Yes, the checking against MK_ZFS and changing the #define to HAVE_ZFS makes this work with WITHOUT_ZFS. Looks good to me.

[trasz]

Yeah, the version with "if !defined()" might be even prettier; mine gives a smaller diff.

In any case - WARNS=5 is fine, 6 would be even better, 0 is too low, imho. ;-)