Page MenuHomeFreeBSD

makefs cast daddr_t to off_t before multiplication
ClosedPublic

Authored by freebsdphab-AX9_cmx.ietfng.org on Dec 3 2020, 3:13 PM.

Details

Summary

Apparently some large-file systems out there, such as my powerpc64le Linux box, define daddr_t as a 32-bit type, which is sad and stymies cross-building disk images. Cast daddr_t to off_t before doing arithmetic that overflows.

Test Plan

Compile and build disk images using cheribuild

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This revision is now accepted and ready to land.Dec 3 2020, 3:17 PM
imp requested changes to this revision.Dec 3 2020, 3:34 PM

What guarantee is there that off_t is going to be better? If we are cross building, wouldn't it be better to ensure we get the right sized data types rather than play whack a mole like this? Daddr_t is a block offset.

usr.sbin/makefs/ffs/buf.h
62 ↗(On Diff #80264)

Wouldn't it be better to add

#ifdef linux
typedef int64_t freebsd_daddr_t
#define daddr_t freebsd_daddr_t
#endif

towards the top of this file?

This revision now requires changes to proceed.Dec 3 2020, 3:34 PM
In D27458#613527, @imp wrote:

What guarantee is there that off_t is going to be better? If we are cross building, wouldn't it be better to ensure we get the right sized data types rather than play whack a mole like this? Daddr_t is a block offset.

It ultimately gets passed to lseek, so if your off_t is 32-bit nothing's going to help you. If we cared about bootstrapping on 32-bit Linux we'd have to enable LFS for lseek to work at which point off_t is then correct again (or explicitly use lseek64 and off64_t).

In D27458#613527, @imp wrote:

What guarantee is there that off_t is going to be better? If we are cross building, wouldn't it be better to ensure we get the right sized data types rather than play whack a mole like this? Daddr_t is a block offset.

It ultimately gets passed to lseek, so if your off_t is 32-bit nothing's going to help you. If we cared about bootstrapping on 32-bit Linux we'd have to enable LFS for lseek to work at which point off_t is then correct again (or explicitly use lseek64 and off64_t).

We _could_ get away with _just_ including the cast part of the diff and still use daddr_t everywhere else if you want a more minimal diff, but you'd still be limited to BLOCKSIZE * 2^32 bytes on such 64-bit Linux systems.

Ok. I'm sold. Forget my suggestion

This revision is now accepted and ready to land.Dec 3 2020, 4:41 PM

Wes, could you write a better commit message that captures that justification please? Alternatively Alex or I can write one for you when committing if that's fine with you.

jrtc27 requested changes to this revision.Dec 14 2020, 1:01 AM

Hm, on reflection, this doesn't quite do what it claims. Many of the arguments to this function come from fsbtodb and there are lots of daddr_t's floating around. Unless you want to purge all of them from existence (which is notionally wrong as they _are_ block numbers) I'd suggest leaving just the cast and dropping the rest of the diff.

This revision now requires changes to proceed.Dec 14 2020, 1:01 AM
freebsdphab-AX9_cmx.ietfng.org retitled this revision from Make makefs use off_t instead of daddr_t to makefs cast daddr_t to off_t before multiplication.
freebsdphab-AX9_cmx.ietfng.org edited the summary of this revision. (Show Details)

Just the casts now, no function or structure type changes. This appears to pass muster locally.

This looks good to me now. Does anyone else have comments?

This revision is now accepted and ready to land.May 30 2021, 3:36 PM