Page MenuHomeFreeBSD

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

Authored by allanjude on Mar 11 2015, 1:02 PM.
Tags
None
Referenced Files
F86667974: D2045.id6398.diff
Sun, Jun 23, 6:01 PM
Unknown Object (File)
Fri, Jun 21, 12:10 PM
Unknown Object (File)
Fri, Jun 21, 11:40 AM
Unknown Object (File)
Thu, Jun 20, 7:04 PM
Unknown Object (File)
Thu, Jun 20, 5:52 PM
Unknown Object (File)
Thu, Jun 20, 5:42 PM
Unknown Object (File)
Thu, Jun 20, 5:37 PM
Unknown Object (File)
Thu, Jun 13, 10:08 AM

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
usr.sbin/fstyp/fstyp.c
65 ↗(On Diff #4867)

so what makes zfs mountable, and ufs not mountable?

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 added inline comments.
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?

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.

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.

Update man page and usage statement

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 edited edge metadata.

docs only reviewed; please fix the nit I pointed out

usr.sbin/fstyp/fstyp.8
123 ↗(On Diff #5441)

please fix this

This revision is now accepted and ready to land.May 18 2015, 6:34 PM
allanjude edited edge metadata.

Feedback from trasz

This revision now requires review to proceed.May 18 2015, 6:41 PM
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?

usr.sbin/fstyp/Makefile
29 ↗(On Diff #5460)

bapt@, I believe.

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.

trasz edited edge metadata.
This revision is now accepted and ready to land.May 19 2015, 4:15 PM
wblock added inline comments.
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 added inline comments.
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 edited edge metadata.
allanjude marked an inline comment as done.

man page updated with feedback from wblock@

This revision now requires review to proceed.May 29 2015, 5:54 AM
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 edited edge metadata.

Feedback from trasz

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"

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

usr.sbin/fstyp/fstyp.8
126 ↗(On Diff #5778)

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

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

usr.sbin/fstyp/fstyp.8
56 ↗(On Diff #5778)

s/provides/providers

Updated with feedback from asomers

trasz edited edge metadata.
This revision is now accepted and ready to land.Jun 16 2015, 1:40 PM
wblock added a reviewer: wblock.

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

This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Jun 19 2015, 9:20 PM
allanjude edited edge metadata.

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

Change WARNS level

This revision now requires review to proceed.Jun 19 2015, 9:21 PM

Erm, what's the problem with WARNS?

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

In D2045#55644, @trasz wrote:

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

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?

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

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,

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.

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

imp added a reviewer: imp.
This revision is now accepted and ready to land.Jun 23 2015, 4:20 PM
This revision was automatically updated to reflect the committed changes.

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