Re: [PATCH] Set size of partition correctly in grub_disk_open()

[Jeroen Dekkers]

At Sat, 8 Jul 2006 23:10:56 +0200,
Yoshinori K. Okuji wrote:
> 
> On Saturday 08 July 2006 22:39, Jeroen Dekkers wrote:
> > But it's a little bit illogical that the size you get from the same
> > disk structure isn't the size of the partition, but the size of
> > something else.
> 
> It is not illogical from my point of view. The disk structure should describe 
> the information on a disk but not on a partition. Your way looks illogical to 
> me.

The illogical thing for me is: You ask to open a partition, you get
back a structure that has half the properties of the partition, and
the other half has the properties of the disk the partition is on.

For me the grub_disk_* seem to just be an abstract interface to either
open a disk or a partition. This just worked all fine, except that
total_sectors that doesn't match what grub_disk_read() does.

> > It's also not really useful: if you're opening a 
> > partition, your are interested in the size of the partition most of
> > the time, not the size of the disk the partition is on.
> 
> No. When you open something, you usually have no interest in the size.

Yes, but those unusual places that do have interest in it want to know
the size of the partition and not the size of the disk that contains
the partition.

> > The AFFS code 
> > already assumes that the total_sectors is the size of the partition
> > and the blocklist code does that too, if you want to allow to read a
> > blocklist from a partition.
> 
> blocklist does not. I fixed this bug some weeks ago.

grub_fs_blocklist_open() has the following:

if (disk->total_sectors < blocks[i].offset + blocks[i].length)
  {
    grub_error (GRUB_ERR_BAD_FILENAME, "beyond the total sectors");
    goto fail;
  }

Given that disk->total_sectors is the number of sectors and disk can
be a partition, this check will allow to read after the end of the
partition.

> I don't remember about AFFS. I haven't proofread the code carefully.

AFAICS it calculates the position of the root block wrong, by assuming
that disk->total_sectors is the size of the partition it's reading.
 
> > Not having total_sectors the size of the partition also makes it
> > impossible to write generic code for both disks and
> > partitions. Everytime you want to get the size of a device, you've to
> > check whether the device is a disk or a partition. This will enlarge
> > the code unnecessary.
> 
> Tell me why you need to know the size. The range check is automatically done 
> by the disk interface, so you won't have to deal with such a check in the 
> filesystem code.

The RAID superblock is located at 64KiB before the end of the device,
which can either be a partition or a whole disk. (For AFFS it seems
that the root block is located in the middle of the partition)
 
> > So I don't really see why total_sectors should be the size of the disk
> > the partition is on instead of the size of the partition.
> 
> Because the disk structure is for disks, and the partition structure is for 
> partitions. Overwriting total_sectors in a disk means that you only lose 
> information. If your concern is only the conditional that checks if a disk 
> contains a partition or not, you can write a functional such as 
> grub_device_get_size.

So why do I get a disk structure when I want to open a partition, and
not a partition structure, if disk structures aren't for partitions?

Jeroen Dekkers