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

[Jeroen Dekkers]

At Sun, 9 Jul 2006 03:11:33 +0200,
Yoshinori K. Okuji wrote:
> > > 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.
> 
> It depends on a case. I sometimes wanted to know disk information even when 
> opening a partition, but I don't remember which case exactly. I must take a 
> look at the code.

I don't see anything like that in the code when I grep for
total_sectors. The only code that is using total_sectors as the size
of the disk is the grub_disk_check_range(), but that function is also
interested in whether the offset/size fits in the partition when
reading from a partition. If total_sectors was the size of the
partition, the two ifs in grub_disk_check_range() could be merged to
just one if for both cases.

> > > 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;
> >   }
> 
> Ah, you meant the blocklist _filesystem_. I misunderstood that you meant the 
> blocklist _command_. Yes, you are right. This is a bug. I haven't tested the 
> blocklist filesystem with a partition, so I didn't know this bug. I will fix 
> it later.
> 
> > > 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.
> 
> I see. It is definitely a bug. I must fix it.

Well, both users of total_sectors are assuming the semantics I'm
proposing, which only reinforces my point that it's the more logical
thing to do.
 
> > 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?
> 
> You get *both*. I assume that the question is the hierarchy. The current 
> design is based on this concept:
> 
> device -> disk -> partition
>       \
>        -> net -> protocol
> 
> I did want to abstract all kinds of devices as just devices, whenever 
> possible. And the real character of a given device is described as attributes 
> to the device (here, "disk" and "net"). In this organization, it is natural 
> to have "partition" under "disk". If this order is reverse, it is very 
> strange to me, because "partition" does not exist without "disk", as the 
> purpose of "partition" is to partition a disk. Am I wrong?

We have both have disks and partitions. As a user of the interface, it
seems useful to me that you can just do a grub_disk_open() (or
grub_device_open(), but that just opens a disk at the moment and
you've to get device->disk and fiddle with that, so it isn't really
different) and get an object back. On that object you can then do a
read, write, get the size, etc. You don't have to care whether that
object is actually a partition or a disk, the code abstracts that
away.

Jeroen Dekkers