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 ↗

(On Diff #4867)

so what makes zfs mountable, and ufs not mountable?

[allanjude]

usr.sbin/fstyp/fstyp.c

65 ↗

(On Diff #4867)

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

22 ↗

(On Diff #4868)

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

24 ↗

(On Diff #4868)

combine with CFLAGS above

usr.sbin/fstyp/fstyp.c

143 ↗

(On Diff #4868)

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

46 ↗

(On Diff #4868)

double checking tabbing

usr.sbin/fstyp/zfs.c

63 ↗

(On Diff #4868)

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

68 ↗

(On Diff #4868)

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

22 ↗

(On Diff #4868)

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 ↗

(On Diff #5440)

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

97 ↗

(On Diff #5441)

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

123 ↗

(On Diff #5441)

please fix this

[allanjude]

Feedback from trasz

[allanjude]

usr.sbin/fstyp/Makefile

29 ↗

(On Diff #5460)

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 ↗

(On Diff #5460)

bapt@, I believe.

[allanjude]

usr.sbin/fstyp/Makefile

29 ↗

(On Diff #5460)

<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 ↗

(On Diff #5460)

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 ↗

(On Diff #5460)

This sentence is not needed (see below).

50 ↗

(On Diff #5460)

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 ↗

(On Diff #5460)

This sentence is not needed.

98 ↗

(On Diff #5460)

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

Include filesystems and devices that cannot be mounted directly by

110 ↗

(On Diff #5460)

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

123 ↗

(On Diff #5460)

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

[allanjude]

usr.sbin/fstyp/fstyp.8

123 ↗

(On Diff #5460)

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 ↗

(On Diff #5769)

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".

102 ↗

(On Diff #5769)

Remove "special"?

[allanjude]

Feedback from trasz

[wblock]

usr.sbin/fstyp/fstyp.8

126 ↗

(On Diff #5774)

"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 ↗

(On Diff #5778)

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 ↗

(On Diff #5778)

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. ;-)