Page MenuHomeFreeBSD

Make it so ufsread.c is linked rather than included.
AbandonedPublic

Authored by benno on Feb 14 2018, 10:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 1, 5:17 PM
Unknown Object (File)
Fri, Jan 13, 9:55 AM
Unknown Object (File)
Dec 11 2022, 7:22 AM
Unknown Object (File)
Nov 29 2022, 6:49 AM
Subscribers

Details

Reviewers
imp
Group Reviewers
sparc64
PowerPC
MIPS
Summary

It used to be that ufsread.c was combined with other boot code by including it using the C preprocessor. This was presumably due to a search for space efficiencies needed by the fact that it needed to fit (with boot1 and btx) into the 8KB available in a BSD disklabel.

Linkers have improved significantly since then and this approach is no longer needed.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 15051
Build 15149: arc lint + arc unit

Event Timeline

benno added reviewers: imp, sparc64, PowerPC, MIPS.
benno set the repository for this revision to rS FreeBSD src repository - subversion.

Needs some work still... Likely will need another pass to make sure it all lines up.

stand/efi/boot1/Makefile
26

We link with libsa here, so this shouldn't be needed.

55–57

This shouldn't be needed because we link with libsa.

stand/i386/boot2/boot2.c
146–148

OK, this changes things.

USE_SMALL_CGBASE is there so that i386 will use smaller than 64-bit disk addresses to do some computations that shouldn't need 64-bit numbers. By doing this, you remove that optimization which has been required to stay small enough in the past.

You've also broken the ability to compile for only UFS1 or UFS2

stand/libsa/ufsread.c
58–61

I'd be tempted to put them into ufsread.h: (a) so that we don't get conflicting decls and (b) the impl matches the decl :)

stand/mips/beri/boot2/Makefile
63

This is likely not needed here. BERI boot2 isn't under the extreme size pressures and if it had it before, it was because it was blindly copied from i386.

stand/sparc64/boot1/boot1.c
51–55

These should be in ufsread.h... The other advantagae is that you aren't declaring forward references to globals just to avoid warnings. Solves the warning, but not the problem the warning is trying to prevent.

benno added inline comments.
stand/efi/boot1/Makefile
26

ufsread.c isn't built into libsa, the code just lives there. We could change this though.

55–57

Same as above.

stand/i386/boot2/boot2.c
146–148

If you check the Makefile I define this when I compile ufsread.c

stand/libsa/ufsread.c
58–61

Yep, I'd be fine with this.

stand/mips/beri/boot2/Makefile
63

I can take it out. I'll double-check with @rwatson and @brooks and co.

stand/sparc64/boot1/boot1.c
51–55

Yeah, makes sense. It just felt like that was the wrong place to put those. Maybe we need another boot.h or something.

stand/efi/boot1/Makefile
26

Please do. It belongs there. It creates a whole crapton of problems to have files that are compiled N different ways in N different places, so if we can put it in libsa, it should go into libsa.

stand/i386/boot2/boot2.c
146–148

Oh, right, the whole reach over and compile it N different ways problem that I so dearly love.

stand/mips/beri/boot2/Makefile
63

beri's boot2 blindly copied way too much from boot2 on i386, and it's been a big hassle, so the few cargo-culted changed because it was like that on x86 we have here the better.

stand/sparc64/boot1/boot1.c
51–55

That's usually called 'stand.h' :)

But we have too many space constrained loaders, so maybe we need to think about the bigger picture.

But in this case, these are interfaces that are required to use ufsread.h. We either need to put them in there, or clearly articulate what every single include of ufsread.h should also include. Since that's a ticklish problem that might have lots of sprawl were we to implement it, I suspect you don't want to do that as part of this review.

Also boot.h is likely the wrong name since it conflicts, I believe, with other files of that name, possibly not located in /stand

stand/mips/beri/boot2/Makefile
63

This seems fine. We've got quite a bit of storage on the FPGA and we don't support 32-bit at all at this point so 64-bit math is fine (if I understand the comments above.)