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
Unknown Object (File)
Feb 8 2024, 6:22 PM
Unknown Object (File)
Jan 31 2024, 3:57 AM
Unknown Object (File)
Jan 31 2024, 3:57 AM
Unknown Object (File)
Jan 31 2024, 3:57 AM
Unknown Object (File)
Jan 31 2024, 3:53 AM
Unknown Object (File)
Jan 31 2024, 3:53 AM
Unknown Object (File)
Jan 31 2024, 3:53 AM
Unknown Object (File)
Jan 31 2024, 3:53 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 Passed
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
usr.sbin/fstyp/fstyp.c
65

so what makes zfs mountable, and ufs not mountable?

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

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.

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.

Update man page and usage statement

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

Feedback from trasz

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

usr.sbin/fstyp/Makefile
29

bapt@, I believe.

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

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

Feedback from trasz

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"

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

usr.sbin/fstyp/fstyp.8
126

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

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