[Toybox] [landley/toybox] Is binary searching length of a file still necessary? (#173)

[Rob Landley]

On 3/1/20 8:06 AM, Park Ju Hyung wrote:
> I've noticed that fdlength() at lib/lib.c uses a very weird method to get the
> file size:

It was because I needed to do that to find the size of a CD ROM in a CD drive.
It only does it as a fallback.

Huh, it SHOULD only do it as a fallback. Why is it commented out?

Annotate says... commit 035f27ae4. Since 2013 and nobody noticed? Sigh.

Looks like I had it commented out to _test_ the fallback, accidentally checked
it in with it still commented out. :P

And to answer the question I left myself in the comment, by looking at the linux
kernel source in block/ioctl.c:

        case BLKGETSIZE:
                size = i_size_read(bdev->bd_inode);
                if ((size >> 9) > ~0UL)
                        return -EFBIG;
                return put_ulong(argp, size >> 9);

Yes, >>9 is 512 bytes. But more to the point:

        case BLKGETSIZE64:
                return put_u64(argp, i_size_read(bdev->bd_inode));

An annotate drill says that's been there since at least 2002 (when its
implementation was moved from another file) so well beyond the 7 years..).

> This breaks mkswap'ing a block device with broken read(2): e.g. a vnswap device.

Breaks how? (You have a write-only device?)

> Granted, vnswap is not a typical block device as it's meant to be exclusively
> used within the kernel, but it's still weird seeing such method still in use.

It should be the fallback if the ioctl fails.

> I don't know the rationale behind using a binary search as it predates the
> original code introduction in commit 055cfcb
> <https://github.com/landley/toybox/commit/055cfcbe5b0534c700b30216f54336b7581f7be4>
> , but SEEK_END would suffice imho(and it fixes vnswap).

In the case of a CDROM I think it's that the ioctl didn't spin up the disk, but
the kernel didn't know how big it was yet if it wasn't spun up? (This was a
while back.)

Rob