Page MenuHomeFreeBSD

Add hammer support for fstyp(8).
Needs ReviewPublic

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

Details

Reviewers
eadler
Group Reviewers
manpages
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
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13334
Build 13567: arc lint + arc unit

Event Timeline

pfg created this revision.Dec 4 2017, 8:40 PM
bcr added a subscriber: bcr.Dec 4 2017, 9:11 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.

pfg added a comment.Dec 4 2017, 10:18 PM
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.

pfg added a reviewer: eadler.Dec 6 2017, 1:21 AM

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

nit: I always forget style(9) but I think this is in the wrong place. also sort.

59

I'd prefer globals to static variables in functions unless we have good reason

61

where is 64 coming from?

65

can we name these constants?

149

pretty sure you can replace most of this with strtok

usr.sbin/fstyp/hammer2.c
36

nit: sort

70

nit: I feel like FreeBSD style frowns on mixing declarations and init like this, though I'm not sure that's a good thing

pfg added a comment.Dec 10 2017, 1:36 AM

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

This is the only part I wrote ;).

danfe added a subscriber: danfe.Dec 10 2017, 8:43 AM
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)?

pfg added a comment.Dec 10 2017, 3:32 PM
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.

danfe added a comment.Dec 10 2017, 4:37 PM
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.

pfg added a comment.Dec 10 2017, 5:03 PM
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