[PATCH] do not require that device is a partition

[PATCH] do not require that device is a partition

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]


Hi!

Devices where the filesystem is in whole disk are common nowadays [1]; I
don't think it's a good idea to require that a filesystem is in a partition
in order to access it.

See attached patch.  Any comments?

[1] for example, debian provides usb images of the debian-installer with a
    fat filesystem and no partitions:
    http://people.debian.org/~joeyh/d-i/images/daily/hd-media/boot.img.gz

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: disk_fs.diff --]
[-- Type: text/x-diff, Size: 580 bytes --]

2007-10-05  Robert Millan  <rmh@aybabtu.com>

	* normal/misc.c (grub_normal_print_device_info): Do not require that
	device is a partition.

diff -ur grub2/normal/misc.c grub2.disk_fs/normal/misc.c
--- grub2/normal/misc.c	2007-07-22 01:32:29.000000000 +0200
+++ grub2.disk_fs/normal/misc.c	2007-10-05 11:55:29.000000000 +0200
@@ -40,7 +40,7 @@
   dev = grub_device_open (name);
   if (! dev)
     grub_printf ("Filesystem cannot be accessed");
-  else if (! dev->disk || ! dev->disk->has_partitions || dev->disk->partition)
+  else
     {
       char *label;
       grub_fs_t fs;

Re: [PATCH] do not require that device is a partition

[Robert Millan]

On Fri, Oct 05, 2007 at 11:59:48AM +0200, Robert Millan wrote:
> -  else if (! dev->disk || ! dev->disk->has_partitions || dev->disk->partition)
> +  else

Actually, the idea behind this check seems to be that if the device is a
disk, we only want to probe for filesystems if it has no partitions.

The problem with this is that filesystems in general don't garantee us that
the first 512 bytes aren't filled with information that, to the MBR partition
table parser, is seen as garbage and results in undefined partition layout.

I think disk->has_partitions as a whole is flawed for this reason.  Should we
get rid of it?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] do not require that device is a partition

[Robert Millan]

On Fri, Oct 05, 2007 at 02:30:08PM +0200, Robert Millan wrote:
> On Fri, Oct 05, 2007 at 11:59:48AM +0200, Robert Millan wrote:
> > -  else if (! dev->disk || ! dev->disk->has_partitions || dev->disk->partition)
> > +  else
> 
> Actually, the idea behind this check seems to be that if the device is a
> disk, we only want to probe for filesystems if it has no partitions.
> 
> The problem with this is that filesystems in general don't garantee us that
> the first 512 bytes aren't filled with information that, to the MBR partition
> table parser, is seen as garbage and results in undefined partition layout.
> 
> I think disk->has_partitions as a whole is flawed for this reason.  Should we
> get rid of it?

Thinking again, from GRUB POV we can't really tell wether the partition table
or the filesystem is right in this situation, so I would suggest to just not
use this in this particular check, but leave has_partitions as is, since it has
its purpose.

Any comments?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] do not require that device is a partition

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]


New patch after IRC discussion with Marco.  code + ChangeLog should be
self-explanatory.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: disk.diff --]
[-- Type: text/x-diff, Size: 1361 bytes --]

2007-10-14  Robert Millan  <rmh@aybabtu.com>

	* normal/misc.c (grub_normal_print_device_info): Do not probe for
	filesystem when dev->disk is unset.
	Do probe for filesystem even when dev->disk->has_partitions is set.
	In case a filesystem is found, always report it.
	In case it isn't, if dev->disk->has_partitions is set, report that
	a partition table was found instead of reporting that no filesystem
	could be identified.

diff -pur grub2/normal/misc.c grub2.disk/normal/misc.c
--- grub2/normal/misc.c	2007-07-22 01:32:29.000000000 +0200
+++ grub2.disk/normal/misc.c	2007-10-14 17:41:57.000000000 +0200
@@ -40,7 +40,7 @@ grub_normal_print_device_info (const cha
   dev = grub_device_open (name);
   if (! dev)
     grub_printf ("Filesystem cannot be accessed");
-  else if (! dev->disk || ! dev->disk->has_partitions || dev->disk->partition)
+  else if (dev->disk)
     {
       char *label;
       grub_fs_t fs;
@@ -49,7 +49,12 @@ grub_normal_print_device_info (const cha
       /* Ignore all errors.  */
       grub_errno = 0;
 
-      grub_printf ("Filesystem type %s", fs ? fs->name : "unknown");
+      if (fs)
+	grub_printf ("Filesystem type %s", fs->name);
+      else if (! dev->disk->has_partitions || dev->disk->partition)
+	grub_printf ("Unknown filesystem");
+      else
+	grub_printf ("Partition table");
 	  
       if (fs && fs->label)
 	{

Re: [PATCH] do not require that device is a partition

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> New patch after IRC discussion with Marco.  code + ChangeLog should be
> self-explanatory.
>
> -- 
> Robert Millan
>
> <GPLv2> I know my rights; I want my phone call!
> <DRM> What use is a phone call, if you are unable to speak?
> (as seen on /.)
>
> 2007-10-14  Robert Millan  <rmh@aybabtu.com>
>
> 	* normal/misc.c (grub_normal_print_device_info): Do not probe for
> 	filesystem when dev->disk is unset.
> 	Do probe for filesystem even when dev->disk->has_partitions is set.
> 	In case a filesystem is found, always report it.
> 	In case it isn't, if dev->disk->has_partitions is set, report that
> 	a partition table was found instead of reporting that no filesystem
> 	could be identified.


"Do not probe for filesystem when dev->disk is unset."

This was already the case, right?  So perhaps this line should be removed?

Feel free to commit it.

--
Marco

Re: [PATCH] do not require that device is a partition

[Robert Millan]

On Mon, Oct 15, 2007 at 09:59:19AM +0200, Marco Gerards wrote:
> 
> "Do not probe for filesystem when dev->disk is unset."
> 
> This was already the case, right?  So perhaps this line should be removed?

No, that was another bug:

17:05 < nyu> we're probing for filesystems on NICs currently
17:06 < nyu> it's one of the entry conditions
17:06 < marco_g> That's wrong :)

> Feel free to commit it.

Done.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)