Paths

Table of Contentst

Add hammer support for fstyp(8).
ClosedPublic
Actions

Authored by pfg on Dec 4 2017, 8:40 PM.

Details

Reviewers

eadler
bcr
cem

Group Reviewers

manpages

Commits

rS356060: sbin/fstyp: recgonize Dragonfly's hammer and hammer2.

Summary

Dragonfly ported fstyp(8) and included support for the hammer filesystem
variants. Bringing back the support to FreeBSD seemed to be an easy task
so I gave it a try.

I did have to bring along two basic headers from Dragonfly and change
things a bit to get it to compile.

Test Plan

It remains untested: I don't really have Dragonfly installed and I can't
generate atest image either.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

pfg created this revision.Dec 4 2017, 8:40 PM

Herald added a reviewer: manpages. · View Herald TranscriptDec 4 2017, 8:40 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B13334: Diff 36205.Dec 4 2017, 8:40 PM

At the top of fstyp.8, you need to bump the .Dd to the date of the commit, since this is a content change.

In D13369#279103, @bcr wrote:

At the top of fstyp.8, you need to bump the .Dd to the date of the commit, since this is a content change.

Yes, I just currently don't know anyone that can test this so I have no idea when (or if) it will be committed.

Perhaps Eitan is interested ...

I did not review the header files which look like straight up copies from DragonFly. Most of comments are nits or style and can be ignored if I'm wrong.

Thanks!

usr.sbin/fstyp/hammer.c
38 ↗	(On Diff #36205)	nit: I always forget style(9) but I think this is in the wrong place. also sort.
59 ↗	(On Diff #36205)	I'd prefer globals to static variables in functions unless we have good reason
61 ↗	(On Diff #36205)	where is `64` coming from?
65 ↗	(On Diff #36205)	can we name these constants?
149 ↗	(On Diff #36205)	pretty sure you can replace most of this with `strtok`
usr.sbin/fstyp/hammer2.c
36 ↗	(On Diff #36205)	nit: sort
70 ↗	(On Diff #36205)	nit: I feel like FreeBSD style frowns on mixing declarations and init like this, though I'm not sure that's a good thing

Interesting ...
I took the C files from DragonfLyBSD as well: I only changed them so that they would compile with the headers placed locally.
The style issues should probably be reported upstream, it is not something we should be fixing..
The logical question is if it works at all. The next question is ... do we want this?

usr.sbin/fstyp/hammer.c
38 ↗	(On Diff #36205)	This is the only part I wrote ;).

In D13369#280745, @pfg wrote:

The logical question is if it works at all. The next question is ... do we want this?

This is the main thing I was wondering about when I saw this DR. Do we ever plan to have this Hammer thing support, like being able to mount their volumes? What is possible usage scenario of this code in FreeBSD right now, without actual mount support? I fear that this code won't be maintained well and might start to bitrot since day one.

P.S. On an unrelated note: why fstyp(8) is not called fstype(8)?

In D13369#280752, @danfe wrote:

In D13369#280745, @pfg wrote:

The logical question is if it works at all. The next question is ... do we want this?

This is the main thing I was wondering about when I saw this DR. Do we ever plan to have this Hammer thing support, like being able to mount their volumes? What is possible usage scenario of this code in FreeBSD right now, without actual mount support? I fear that this code won't be maintained well and might start to bitrot since day one.

I think it would eventually make sense to have support for hammer2, and then this would make perfect sense. It doesn't hurt to know more of what is installed in the hard disk.

This said, we can keep it in code review until the time comes when its more useful.

P.S. On an unrelated note: why fstyp(8) is not called fstype(8)?

Solaris and linux use fstyp.

In D13369#280745, @pfg wrote:

Interesting ...
I took the C files from DragonfLyBSD as well: I only changed them so that they would compile with the headers placed locally.
The style issues should probably be reported upstream, it is not something we should be fixing..

Alright.

The logical question is if it works at all.

I could test this out, but it'll take some time. My current dev setup is all VMs and my fbsd time is limited.

The next question is ... do we want this?

IMHO yes.

In D13369#280775, @eadler wrote:

In D13369#280745, @pfg wrote:

The next question is ... do we want this?

IMHO yes.

I still think that bringing in such amount of code for an otherwise unsupported filesystem is premature. As it was said, we can (and should) keep it in code review until the time comes when it's more useful.

In D13369#280781, @danfe wrote:

In D13369#280775, @eadler wrote:

In D13369#280745, @pfg wrote:

The next question is ... do we want this?

IMHO yes.

I still think that bringing in such amount of code for an otherwise unsupported filesystem is premature. As it was said, we can (and should) keep it in code review until the time comes when it's more useful.

In all honesty ... it is not huge and it is self contained. Furthermore it is maintained in DragonFlyBSD. But there is no hurry. let's wait to see if it works first ;).

trasz added a subscriber: trasz.Jun 22 2018, 11:14 AM

Since this is an import from DragonflyBSD, any changes to the man page should be sent upstream. So far, I'm fine with it and won't block the integration from the doc side of things.

This revision is now accepted and ready to land.Dec 20 2018, 8:24 PM

cem@ has been adding some interesting support to fstyp so perhaps he may want to review or even take over.

Also upstream has made some small changes, but I have no time to catch up:

https://gitweb.dragonflybsd.org/dragonfly.git/history/HEAD:/usr.sbin/fstyp

Regarding it not being mountable at the moment: that’s what the ‘-u’ option (and the corresponding flag in the file systems list) is for.

No objection. Most of the imported code is headers, which won't result in a huge binary. The small C program is a handful of tiny functions, seems unobjectionable to me. I am not super familiar with hammer and didn't read this code thoroughly, but given most of it comes verbatim from dfly I'm willing to trust their ability to recognize their own FS.