Page MenuHomeFreeBSD
Differential D2045

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

Authored by allanjude on Mar 11 2015, 1:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 6:21 PM
Unknown Object (File)
Thu, Nov 14, 12:56 PM
Unknown Object (File)
Wed, Nov 13, 3:05 AM
Unknown Object (File)
Wed, Nov 6, 3:18 PM
Unknown Object (File)
Tue, Nov 5, 10:00 AM
Unknown Object (File)
Mon, Oct 28, 7:23 AM
Unknown Object (File)
Sun, Oct 27, 5:52 PM
Unknown Object (File)
Sun, Oct 27, 12:55 AM
View All Files
Subscribers
asomers
dan.mcgregor_usask.ca
eadler
imp
wblock

Details

Reviewers
eadler
emaste
trasz
wblock
imp
asomers
Group Reviewers
manpages
Commits
rS284728: Switch fstyp build to toggle ZFS support on WITHOUT_ZFS rather than WITHOUT_CDDL
rS284589: Add the ability to detect ZFS and GELI encrypted file systems to fstyp(8)
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 Passed
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
imp added inline comments.Apr 17 2015, 4:28 AM
usr.sbin/fstyp/fstyp.c
65

so what makes zfs mountable, and ufs not mountable?

allanjude added inline comments.Apr 17 2015, 4:32 AM
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 added a subscriber: eadler.Apr 17 2015, 5:06 AM
eadler added inline comments.
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 added a comment.Apr 30 2015, 1:05 PM

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.

eadler added a reviewer: eadler.May 17 2015, 6:48 PM
allanjude updated this revision to Diff 5440.May 17 2015, 7:55 PM

Makefile fixes

allanjude marked 4 inline comments as done.May 17 2015, 7:57 PM

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 updated this revision to Diff 5441.May 17 2015, 10:06 PM

Update man page and usage statement

Herald added a reviewer: manpages. · View Herald TranscriptMay 17 2015, 10:06 PM
trasz added inline comments.May 18 2015, 1:36 PM
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 accepted this revision.May 18 2015, 6:34 PM
eadler edited edge metadata.

docs only reviewed; please fix the nit I pointed out

usr.sbin/fstyp/fstyp.8
128

please fix this

This revision is now accepted and ready to land.May 18 2015, 6:34 PM
allanjude marked 2 inline comments as done.May 18 2015, 6:41 PM
allanjude updated this revision to Diff 5460.May 18 2015, 6:41 PM
allanjude edited edge metadata.

Feedback from trasz

This revision now requires review to proceed.May 18 2015, 6:41 PM
allanjude added inline comments.May 18 2015, 6:43 PM
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 added inline comments.May 19 2015, 10:13 AM
usr.sbin/fstyp/Makefile
29

bapt@, I believe.

allanjude added inline comments.May 19 2015, 3:39 PM
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.

trasz accepted this revision.May 19 2015, 4:15 PM
trasz edited edge metadata.
This revision is now accepted and ready to land.May 19 2015, 4:15 PM
wblock added a subscriber: wblock.May 19 2015, 5:35 PM
wblock added inline comments.
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 marked 6 inline comments as done.May 29 2015, 5:53 AM
allanjude added inline comments.
usr.sbin/fstyp/fstyp.8
126

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

allanjude updated this revision to Diff 5769.May 29 2015, 5:54 AM
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
trasz added inline comments.May 29 2015, 6:08 AM
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 updated this revision to Diff 5774.May 29 2015, 2:19 PM
allanjude edited edge metadata.

Feedback from trasz

wblock added inline comments.May 29 2015, 7:41 PM
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 updated this revision to Diff 5778.May 29 2015, 8:18 PM

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

emaste added inline comments.May 29 2015, 8:20 PM
usr.sbin/fstyp/fstyp.8
126

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

trasz added a comment.Jun 13 2015, 11:42 AM

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

asomers added a subscriber: asomers.Jun 13 2015, 2:57 PM
asomers added inline comments.Jun 15 2015, 7:04 PM
usr.sbin/fstyp/fstyp.8
56

s/provides/providers

allanjude updated this revision to Diff 6228.Jun 16 2015, 3:21 AM

Updated with feedback from asomers

allanjude added a comment.Jun 16 2015, 3:21 AM

Is this ready for commit now?

trasz accepted this revision.Jun 16 2015, 1:40 PM
trasz edited edge metadata.
This revision is now accepted and ready to land.Jun 16 2015, 1:40 PM
asomers accepted this revision.Jun 16 2015, 3:13 PM
asomers added a reviewer: asomers.
wblock accepted this revision.Jun 18 2015, 2:59 PM
wblock added a reviewer: wblock.

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

Closed by commit rS284589: Add the ability to detect ZFS and GELI encrypted file systems to fstyp(8) (authored by allanjude). · Explain WhyJun 19 2015, 5:09 AM
This revision was automatically updated to reflect the committed changes.
allanjude reopened this revision.Jun 19 2015, 9:20 PM
This revision is now accepted and ready to land.Jun 19 2015, 9:20 PM
allanjude updated this revision to Diff 6337.Jun 19 2015, 9:21 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
trasz added a comment.Jun 20 2015, 8:59 AM

Erm, what's the problem with WARNS?

trasz added a comment.Jun 20 2015, 10:02 AM

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

allanjude added a comment.Jun 20 2015, 3:13 PM
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 added a comment.Jun 20 2015, 3:14 PM
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 added a comment.Jun 20 2015, 4:43 PM

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 added a comment.Jun 20 2015, 5:29 PM
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 added a comment.Jun 21 2015, 8:31 AM

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 added a comment.Jun 21 2015, 3:10 PM
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 added a subscriber: dan.mcgregor_usask.ca.Jun 23 2015, 2:34 PM
dan.mcgregor_usask.ca added a comment.Jun 23 2015, 2:45 PM

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

imp accepted this revision.Jun 23 2015, 4:20 PM
imp added a reviewer: imp.
This revision is now accepted and ready to land.Jun 23 2015, 4:20 PM
Closed by commit rS284728: Switch fstyp build to toggle ZFS support on WITHOUT_ZFS rather than WITHOUT_CDDL (authored by allanjude). · Explain WhyJun 23 2015, 4:34 PM
This revision was automatically updated to reflect the committed changes.
trasz added a comment.Jun 25 2015, 8:24 PM

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

Revision Contents
Changeset List

PathSize
usr.sbin/
fstyp/
33 lines
4 lines
48 lines
28 lines
76 lines
73 lines

Diff 5774

View Options

usr.sbin/fstyp/Makefile

Loading...
View Options

usr.sbin/fstyp/fstyp.h

Loading...
View Options

usr.sbin/fstyp/fstyp.8

Loading...
View Options

usr.sbin/fstyp/fstyp.c

Loading...
View Options

usr.sbin/fstyp/geli.c

Loading...
View Options

usr.sbin/fstyp/zfs.c

Loading...