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

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

[Jeroen Dekkers]

Hi,

When we open a partition grub_disk_open() sets disk->total_sectors to
the size of the disk, not the size of the partition. We also don't
check whether we grub_partition_probe() actually finds a
partition. See the patch below.

Jeroen Dekkers


2006-07-05  Jeroen Dekkers  <jeroen@dekkers.cx>

	* kern/disk.c (grub_disk_open): Check whether
	grub_partition_probe() finds a partition. Set disk->total_sectors
	to the size of the partition if we open a partition.

Index: kern/disk.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/disk.c,v
retrieving revision 1.13
diff -u -p -r1.13 disk.c
--- kern/disk.c	4 Jun 2006 15:56:54 -0000	1.13
+++ kern/disk.c	5 Jul 2006 20:51:18 -0000
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2004  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2004,2006  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -265,7 +265,16 @@ grub_disk_open (const char *name)
   disk->dev = dev;
 
   if (p)
-    disk->partition = grub_partition_probe (disk, p + 1);
+    {
+      disk->partition = grub_partition_probe (disk, p + 1);
+      if (! disk->partition)
+	{
+	  grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no such partition");
+	  goto fail;
+	}
+      
+      disk->total_sectors = grub_partition_get_len (disk->partition);
+    }
 
   /* The cache will be invalidated about 2 seconds after a device was
      closed.  */

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

[Yoshinori K. Okuji]

Hi Jeroen,

On Wednesday 05 July 2006 22:54, Jeroen Dekkers wrote:
>    if (p)
> -    disk->partition = grub_partition_probe (disk, p + 1);
> +    {
> +      disk->partition = grub_partition_probe (disk, p + 1);
> +      if (! disk->partition)
> +	{
> +	  grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no such partition");
> +	  goto fail;
> +	}

I agree with this part.

> +
> +      disk->total_sectors = grub_partition_get_len (disk->partition);
> +    }

But I don't agree with this part. The member "total_sectors" must describe the 
size of the whole disk but not of the partition. This is because GRUB 
accesses partitions through the disk interface. When accessing a partition, 
GRUB tweaks offsets by the start sector of the partition. If not, it is 
another bug.

Thanks,
Okuji

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

[Jeroen Dekkers]

At Sat, 8 Jul 2006 18:07:44 +0200,
Yoshinori K. Okuji wrote:
> 
> Hi Jeroen,
> 
> On Wednesday 05 July 2006 22:54, Jeroen Dekkers wrote:
> > +
> > +      disk->total_sectors = grub_partition_get_len (disk->partition);
> > +    }
> 
> But I don't agree with this part. The member "total_sectors" must describe the 
> size of the whole disk but not of the partition. This is because GRUB 
> accesses partitions through the disk interface. When accessing a partition, 
> GRUB tweaks offsets by the start sector of the partition. If not, it is 
> another bug.

grub_disk_check_range() increases the sector read so sector 0 is the
first sector of the partition. So if you open a partition, you will
have to read from 0 until the length of the partition, and that will
give you the partition. Now I'm reading the code again I see that the
"out of disk" check needs to be adapted to this change too.

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'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. 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.

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.

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.

Jeroen Dekkers

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

[Yoshinori K. Okuji]

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.

> 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.

> 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.

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

> 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.

> 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.

Okuji 

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

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

[Yoshinori K. Okuji]

On Sunday 09 July 2006 01:35, Jeroen Dekkers wrote:
> 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.

I don't see this. When you open a partiton or a disk, you get a device but not 
a disk... don't you?

> 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.

Because, for me, a partition is an attribute for a disk.

> > > 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.

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.

> > 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.

> 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)

OK.

> 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?

Okuji

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

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

[Yoshinori K. Okuji]

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.

BTW, that's why I recommend studying ruby to understand GRUB better. You can 
learn similar things with python, c++, php, etc., but there are very few 
languages with pure object-orientation. For example, python programmers tend 
to access attributes in an object directly from time to time, because the 
language is not very strict. In ruby, accessors are obligatory.

If you don't still agree with me, it's the end of the discussion.

Okuji

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

[Tomáš Ebenlendr]

Maybe Jeroen (and me) thinks that a 'disk' and 'partition' does not
differ, as  there can be some strange disk with filesystem directly on it
(some flash memories, etc.) and also partitions containing another
partitions. Standard terminology typically defines 'disk' as "root"
partition. I.e. its parent is 'device' instead of partition.

But I haven't seen the code, so this is only guess or proposal how things
can be.

-- 
                                            Tomas 'Ebi' Ebenlendr
                                            http://get.to/ebik

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

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

[Yoshinori K. Okuji]

On Sunday 09 July 2006 23:37, Jeroen Dekkers wrote:
> 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...

As small as possible, but as less ugly as possible. I have recognized what 
happened with GRUB Legacy which only concentrated on how small. So I do not 
want to make the same mistake again.

> 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.

Having the same semantics does not imply the same structure. I strongly 
believe that we should keep the hierarchy of device components in raw data, 
so that we can get full information arbitrarily.

> 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.

IMO, LVM should create a virtual disk, as you did for RAID. This fits into my 
taste.

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

Yes, please.

Okuji