BeFS support for fstyp command

[miguel_gocobachi.dev]

Summary

A simple support for detecting BeFS (BeOS) filesystem with fstyp cli command

Test Plan

make tests

[miguel_gocobachi.dev]

I am debating of using the name "bfs" or "befs", the Linux kernel has two filesystems with those names, and the second one is used for the BeOS filesystem, which is this case.

[0mp]

usr.sbin/fstyp/Makefile

7

bfs.c is out of order

usr.sbin/fstyp/bfs.c

2 ↗

(On Diff #87936)

It's not needed anymore.

Also, you may consider adding SPDX-License-Identifier: BSD-2-Clause-FreeBSD.

43 ↗

(On Diff #87936)

style(9)

52 ↗

(On Diff #87936)

style(9)

usr.sbin/fstyp/tests/Makefile

15

Could you move it to the top to preserve the alphabetical order?

usr.sbin/fstyp/tests/fstyp_test.sh

291

This seems to be slightly out of order.

[miguel_gocobachi.dev]

0mp review changes

[freebsd_igalic.co]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

why not return bool here?

[miguel_gocobachi.dev]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

is it the standard? I am complete new collaborating with C code in an open source project, so I want to learn what is the best for FreeBSD src.

[freebsd_igalic.co]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

it is, and we use it. inconsistently https://reviews.freebsd.org/D29659 😅

[miguel_gocobachi.dev]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

I guess it will require more changes in all the files and the fstyp.c, I found this gem:

error = fstyp_f(fp, label, sizeof(label));

if (error == 0)

[freebsd_igalic.co]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

if you have time and motivating you could just ignore my comment for now and instead do a cleanup revision

[miguel_gocobachi.dev]

usr.sbin/fstyp/bfs.c

45 ↗

(On Diff #87941)

thank you, I do have time and motivation! I will keep in mind this in future contributions, I would like to add the BFS support for the VFS/FS as well later on.

[pfg]

FWIW, I'd like to see OpenBFS (haiku OS) supported some day on FreeBSD, so this support is welcome.

[pfg]

usr.sbin/fstyp/Makefile

7

Where is bfs.c? I don't see it in the revision anymore.

[miguel_gocobachi.dev]

usr.sbin/fstyp/Makefile

7

https://reviews.freebsd.org/D29917#change-fk9UBYmA71xW I applied some style(9) recommended by @0mp

[pfg]

LGTM, but ultimately cem@ is the local expert.

usr.sbin/fstyp/bfs.c

37 ↗

(On Diff #87941)

FWIW, there should be a <TAB> after #define. I can't check if that is a space or a tab, but it¿s a common mistake.

[miguel_gocobachi.dev]

style(9) applied and bfs test image fixed

[miguel_gocobachi.dev]

changed the name to befs following the linux implementation

[pfg]

We don't have the naming conflict here, but befs is OK.
FWIW, there is a fuse version which would be useful before considering a kernel version.

[pfg]

usr.sbin/fstyp/befs.c

29

This line is not needed, or accurate., since you are not Berkeley.

65

Too many white lines in this function, IMHO. A blank line between declarations and code is enough.

[yuripv]

usr.sbin/fstyp/befs.c

55

wonder if we should provide a define (e.g. BFS_SUPER_BLOCK_OFFSET) for 512.

62

why bzero() the buffer if we are going to overwrite with nul-terminated string anyway?

63

no magic is needed for the 3rd argument, it should be exactly the size of provided buffer

[yuripv]

usr.sbin/fstyp/befs.c

63

oh, we can't expect volume->name to be nul-terminated, then it should be MIN(size, BFS_OS_NAME_LENGTH + 1) so that we copy all of it and null-terminate?

[miguel_gocobachi.dev]

In D29917#672220, @pfg wrote:

We don't have the naming conflict here, but befs is OK.
FWIW, there is a fuse version which would be useful before considering a kernel version.

Thank you. I will go for the name BeFS because I noticed other things using this name, like sysutils/bfs or other operating systems using bfs for something else, so to make it clear, BeFS sounds good to me.

[miguel_gocobachi.dev]

usr.sbin/fstyp/befs.c

63

I want it to be safe, but then using this way I don't really need to rtrim(), right?

[danfe]

@miguel_gocobachi.dev wrote:

Thank you. I will go for the name BeFS because I noticed other things using this name

Yes please. BFS is way too generic and we had experienced name clashes in the past because they were too short. Even better if it could be done consistently, i.e. applied to macros like BFS_OS_NAME_LENGTH and BFS_SUPER_BLOCK_MAGIC1.

[pfg]

In D29917#672452, @danfe wrote:

@miguel_gocobachi.dev wrote:

Thank you. I will go for the name BeFS because I noticed other things using this name

Yes please. BFS is way too generic and we had experienced name clashes in the past because they were too short. Even better if it could be done consistently, i.e. applied to macros like BFS_OS_NAME_LENGTH and BFS_SUPER_BLOCK_MAGIC1.

No. those are documented in Practical File System Design with the Be File System, and we should match their definition. Section 4.8 recommends B_OS_NAME_LENGTH, Alternatively we could use the Haiku definitions.

BTW, it would be nice if we could use other fields from the superblock, but I have no time to investigate whatever other fstype filesystems use.

[miguel_gocobachi.dev]

In D29917#672536, @pfg wrote:

In D29917#672452, @danfe wrote:

@miguel_gocobachi.dev wrote:

Thank you. I will go for the name BeFS because I noticed other things using this name

Yes please. BFS is way too generic and we had experienced name clashes in the past because they were too short. Even better if it could be done consistently, i.e. applied to macros like BFS_OS_NAME_LENGTH and BFS_SUPER_BLOCK_MAGIC1.

No. those are documented in Practical File System Design with the Be File System, and we should match their definition. Section 4.8 recommends B_OS_NAME_LENGTH, Alternatively we could use the Haiku definitions.

BTW, it would be nice if we could use other fields from the superblock, but I have no time to investigate whatever other fstype filesystems use.

Agree, the validation or identification of the file system needs to be with one or two magic fields, since it is just fstyp I though one is enough, it is not altering the fs so. And for the naming Haiku uses BFS and not exactly following the naming of the PDF, not strictly as you mentioned.

[rpokala]

usr.sbin/fstyp/befs.c

42

Since this file is befs, but these symbols are BFS, perhaps add a comment explaining why they are BFS instead of BEFS?

usr.sbin/fstyp/fstyp.8

45

s/BFS/BeFS/

usr.sbin/fstyp/tests/fstyp_test.sh

32

s/befs/BeFS/

[miguel_gocobachi.dev]

naming convention and clean up code

[miguel_gocobachi.dev]

naming convention and clean up code

[danfe]

@pfg wrote:

No. those are documented in Practical File System Design with the Be File System, and we should match their definition.

Ah, if they come from upstream then yes, we probably should not deviate.

[miguel_gocobachi.dev]

Naming convention applied

[miguel_gocobachi.dev]

Naming convention applied

[miguel_gocobachi.dev]

BeFS detection support for fstyp

[pstef]

usr.sbin/fstyp/befs.c

43

Should we cast this constant expression to magic1's type int32_t? (Should the type be int32_t in any case?)
This is not a request for change.

[gbe]

LGTM for the man page part.

[emaste]

@trasz or @pstef will you shepherd this in?

[miguel_gocobachi.dev]

usr.sbin/fstyp/befs.c

43

The API documentation is defined this way, and every other system that implemented it are using this way as constant and only defining the int32_t at the structs.

[pstef]

How do I obtain befs.img.bz2? I can see it neither in tree nor in this patch.

[miguel_gocobachi.dev]

In D29917#711373, @pstef wrote:

How do I obtain befs.img.bz2? I can see it neither in tree nor in this patch.

Ah it does not show up on the diff when downloading it but, I can see it is listed here below, specifically here: https://reviews.freebsd.org/F20835718