Page MenuHomeFreeBSD

BeFS support for fstyp command
ClosedPublic

Authored by miguel_gocobachi.dev on Apr 22 2021, 7:32 AM.

Details

Summary

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

Test Plan

make tests

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38909
Build 35798: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 requested changes to this revision.Apr 22 2021, 7:38 AM
0mp added a subscriber: 0mp.
0mp added inline comments.
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
14

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

usr.sbin/fstyp/tests/fstyp_test.sh
289

This seems to be slightly out of order.

This revision now requires changes to proceed.Apr 22 2021, 7:38 AM
me_igalic.co added inline comments.
usr.sbin/fstyp/bfs.c
45 ↗(On Diff #87941)

why not return bool here?

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.

usr.sbin/fstyp/bfs.c
45 ↗(On Diff #87941)

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

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)

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

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.

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

usr.sbin/fstyp/Makefile
7

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

miguel_gocobachi.dev added inline comments.
usr.sbin/fstyp/Makefile
7

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

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 marked an inline comment as done.

style(9) applied and bfs test image fixed

changed the name to befs following the linux implementation

miguel_gocobachi.dev retitled this revision from BFS support for fstyp command to BeFS support for fstyp command.Apr 24 2021, 6:26 AM
miguel_gocobachi.dev edited the summary of this revision. (Show Details)

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.

usr.sbin/fstyp/befs.c
28

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

64

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

yuripv added inline comments.
usr.sbin/fstyp/befs.c
54

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

61

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

62

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

usr.sbin/fstyp/befs.c
62

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?

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.

usr.sbin/fstyp/befs.c
62

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

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.

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.

In D29917#672536, @pfg 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 added inline comments.
usr.sbin/fstyp/befs.c
41

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
31

s/befs/BeFS/

naming convention and clean up code

naming convention and clean up code

@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.

pstef added a subscriber: pstef.
pstef added inline comments.
usr.sbin/fstyp/befs.c
42

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 added a subscriber: gbe.

LGTM for the man page part.

miguel_gocobachi.dev added inline comments.
usr.sbin/fstyp/befs.c
42

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.

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Aug 17 2021, 4:16 PM
This revision was automatically updated to reflect the committed changes.