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

[Jeroen Dekkers]

At Sun, 9 Jul 2006 15:29:17 +0200,
Yoshinori K. Okuji wrote:
> 
> On Sunday 09 July 2006 14:01, Jeroen Dekkers wrote:
> > 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.
> 
> Probably the idea was only in my mind. There are so many things that are not 
> implemented yet!
> 
> > 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.
> 
> No. I don't think we can agree on this, since you don't see my point.
> 
> GRUB is based on an object-oriented design. In object-oriented programming, 
> how attributes are stored is an implementation detail, so attributes should 
> not be accessed directly from the outside. On the other hand, it is sometimes 
> more convenient or just faster to access attributes directly. In this sense, 
> the current code in GRUB is a mixture.
> 
> However, my wish is to enforce the object-oriented paradigm as much as 
> possible. What should the disk structure store as information? Of course, 
> about the disk itself. What should the partition structure as information? Of 
> course, about the partition itself. What should total_sectors store? Of 
> course, about the disk, since it is a part of the disk structure. Is it 
> inconvenient? Then, define an accessor appropriately.
> 
> Your proposal simply breaks the rule of object-encapsulation in 
> object-oriented programming, so I never agree with you about this.

Well, if you consider the total_sectors a private variable, and you
want to have accessor functions for accessing it, then I can
understand your point a bit, but such things will make the kernel
bigger and I thought it was a goal to keep it as small as possible...

I was however thinking about total_sectors as a public variable that
gives the size of the device being opened. I don't really think a
partition is an attribute of a disk (all the partitions might be an
attribute of the disk, but just not a random one). I think it makes
more sense to consider a partition an object of the same class as a
disk, given that they have the same semantics. 

We might have to refactor this code for LVM too, if we consider LVM to
be an advanced partition table. I haven't really looked into
implementing LVM yet, but it's more like a partition table than a
disk. It might make sense to allow people to access it using
(volumegroup, volumename) syntax, like (hardisk, partitionnumber). You
can't just add offsets in grub_disk_check_range like you can with DOS
partitions however. But maybe implementing LVM by adding a new kind of
disk device might be better.

Anyway, do you want me to write a grub_disk_get_size() function?

Jeroen Dekkers