[PATCH] skip over invalid BSD partitions

[PATCH] skip over invalid BSD partitions

[Felix Zielcke]

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

On invalid BSD partitions partmap/pc.c stops with reading the partitons,
so grub doestn't know anything about the ones behind it.
Attached patch fixes this and displays also the partition number not
only the invalid magic it encounters

2008-07-31  Felix Zielcke  <fzielcke@z-51.de>

        * partmap/pc.c (pc_partition_map_iterate): Skip over invalid BSD partitions
	or if there's no space for the disk label and print the partition number on a 
	invalid magic

[-- Attachment #2: skip_invalid_bsd_parts.diff --]
[-- Type: text/x-patch, Size: 1155 bytes --]

Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(Revision 1753)
+++ partmap/pc.c	(Arbeitskopie)
@@ -160,9 +160,11 @@
 		{
 		  /* Check if the BSD label is within the DOS partition.  */
 		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "no space for disk label");
-
+		    {
+		      grub_error (GRUB_ERR_BAD_PART_TABLE,
+				  "no space for disk label");
+		      continue;
+		    }
 		  /* Read the BSD label.  */
 		  if (grub_disk_read (&raw,
 				      (p.start
@@ -175,10 +177,12 @@
 		  /* Check if it is valid.  */
 		  if (label.magic
 		      != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "invalid disk label magic 0x%x",
-				       label.magic);
-
+		    {
+		      grub_error (GRUB_ERR_BAD_PART_TABLE,
+				  "invalid disk label magic 0x%x on partition %d",
+				  label.magic,p.index);
+		      continue;
+		    }
 		  for (pcdata.bsd_part = 0;
 		       pcdata.bsd_part < grub_cpu_to_le16 (label.num_partitions);
 		       pcdata.bsd_part++)

Re: [PATCH] skip over invalid BSD partitions

[Marco Gerards]

Felix Zielcke <fzielcke@z-51.de> writes:

> On invalid BSD partitions partmap/pc.c stops with reading the partitons,
> so grub doestn't know anything about the ones behind it.
> Attached patch fixes this and displays also the partition number not
> only the invalid magic it encounters
>
> 2008-07-31  Felix Zielcke  <fzielcke@z-51.de>
>
>         * partmap/pc.c (pc_partition_map_iterate): Skip over invalid BSD partitions
> 	or if there's no space for the disk label and print the partition number on a 
> 	invalid magic

When does this occur?

> Index: partmap/pc.c
> ===================================================================
> --- partmap/pc.c	(Revision 1753)
> +++ partmap/pc.c	(Arbeitskopie)
> @@ -160,9 +160,11 @@
>  		{
>  		  /* Check if the BSD label is within the DOS partition.  */
>  		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
> -		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
> -				       "no space for disk label");
> -
> +		    {
> +		      grub_error (GRUB_ERR_BAD_PART_TABLE,
> +				  "no space for disk label");
> +		      continue;
> +		    }

If you continue as no error occured, why do you throw an error?

--
Marco

Re: [PATCH] skip over invalid BSD partitions

[Felix Zielcke]

Am Freitag, den 01.08.2008, 12:40 +0200 schrieb Marco Gerards:

> When does this occur?
> 
That's the Debian bugreport for it:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=478238

It's very easy to reproduce.
I never used any *BSD flavour just Linux so I even didn't know about
these BSD subpartitions or whatever these are.
I made a new partition table with Linux fdisk
Set the first partition to type 0x6a for OpenBSD
then the second one to type 0x83 for Linux
formated 2. with ext2
tested it with grub-probe and grub-emu 
It failed to detect it because the for() stops reading further
partitions if the BSD one is invalid.


> 
> If you continue as no error occured, why do you throw an error?
> 

I thought it would be a good idea to give the user a warning that
something is wrong with that partition which has a BSD type.
At least the return is wrong see above.

Re: [PATCH] skip over invalid BSD partitions

[Robert Millan]

On Fri, Aug 01, 2008 at 12:40:17PM +0200, Marco Gerards wrote:
> > -		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
> > -				       "no space for disk label");
> > -
> > +		    {
> > +		      grub_error (GRUB_ERR_BAD_PART_TABLE,
> > +				  "no space for disk label");
> > +		      continue;
> > +		    }
> 
> If you continue as no error occured, why do you throw an error?

Uhm nobody's going to handle this error.  The caller will simply see that
some partitions are not processed.

How about using grub_dprintf instead?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] skip over invalid BSD partitions

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Fri, Aug 01, 2008 at 12:40:17PM +0200, Marco Gerards wrote:
>> > -		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
>> > -				       "no space for disk label");
>> > -
>> > +		    {
>> > +		      grub_error (GRUB_ERR_BAD_PART_TABLE,
>> > +				  "no space for disk label");
>> > +		      continue;
>> > +		    }
>> 
>> If you continue as no error occured, why do you throw an error?
>
> Uhm nobody's going to handle this error.  The caller will simply see that
> some partitions are not processed.
>
> How about using grub_dprintf instead?

Agreed.

--
Marco

Re: [PATCH] skip over invalid BSD partitions

[Felix Zielcke]

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

Am Dienstag, den 05.08.2008, 12:16 +0200 schrieb Marco Gerards:
> Robert Millan <rmh@aybabtu.com> writes:

> >
> > How about using grub_dprintf instead?
> 
> Agreed.
> 

Attached.
I hope I get soon used to your changelog :)


2008-08-05  Felix Zielcke  <fzielcke@z-51.de>

	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
	magic or if there's no space left, use grub_dprintf to issue a warning.


[-- Attachment #2: skip_invalid_bsd_parts.diff --]
[-- Type: text/x-patch, Size: 1134 bytes --]

Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(Revision 1770)
+++ partmap/pc.c	(Arbeitskopie)
@@ -160,9 +160,10 @@
 		{
 		  /* Check if the BSD label is within the DOS partition.  */
 		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "no space for disk label");
-
+		    {
+		      grub_dprintf ("partition","no space for disk label\n");
+		      continue;
+		    }
 		  /* Read the BSD label.  */
 		  if (grub_disk_read (&raw,
 				      (p.start
@@ -175,10 +176,12 @@
 		  /* Check if it is valid.  */
 		  if (label.magic
 		      != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "invalid disk label magic 0x%x",
-				       label.magic);
-
+		    {
+		      grub_dprintf ("partition",
+				    "invalid disk label magic 0x%x on partition %d\n"
+				    label.magic,p.index);
+		      continue;
+		    }
 		  for (pcdata.bsd_part = 0;
 		       pcdata.bsd_part < grub_cpu_to_le16 (label.num_partitions);
 		       pcdata.bsd_part++)

Re: [PATCH] skip over invalid BSD partitions

[Felix Zielcke]

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

Am Dienstag, den 05.08.2008, 12:49 +0200 schrieb Felix Zielcke:
> Am Dienstag, den 05.08.2008, 12:16 +0200 schrieb Marco Gerards:
> > Robert Millan <rmh@aybabtu.com> writes:
> 
> > >
> > > How about using grub_dprintf instead?
> > 
> > Agreed.
> > 
> 
> Attached.

I forgot 2 little spaces ;)

> I hope I get soon used to your changelog :)
> 
> 
2008-08-05  Felix Zielcke  <fzielcke@z-51.de>

	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
	magic or if there's no space left, use grub_dprintf to issue a warning.


[-- Attachment #2: skip_invalid_bsd_parts.diff --]
[-- Type: text/x-patch, Size: 1136 bytes --]

Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(Revision 1770)
+++ partmap/pc.c	(Arbeitskopie)
@@ -160,9 +160,10 @@
 		{
 		  /* Check if the BSD label is within the DOS partition.  */
 		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "no space for disk label");
-
+		    {
+		      grub_dprintf ("partition", "no space for disk label\n");
+		      continue;
+		    }
 		  /* Read the BSD label.  */
 		  if (grub_disk_read (&raw,
 				      (p.start
@@ -175,10 +176,12 @@
 		  /* Check if it is valid.  */
 		  if (label.magic
 		      != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "invalid disk label magic 0x%x",
-				       label.magic);
-
+		    {
+		      grub_dprintf ("partition",
+				    "invalid disk label magic 0x%x on partition %d\n"
+				    label.magic, p.index);
+		      continue;
+		    }
 		  for (pcdata.bsd_part = 0;
 		       pcdata.bsd_part < grub_cpu_to_le16 (label.num_partitions);
 		       pcdata.bsd_part++)

Re: [PATCH] skip over invalid BSD partitions

[Marco Gerards]

Felix Zielcke <fzielcke87@googlemail.com> writes:

> 2008-08-05  Felix Zielcke  <fzielcke@z-51.de>
>
> 	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
> 	magic or if there's no space left, use grub_dprintf to issue a warning.

Looks fine.

> +		    {
> +		      grub_dprintf ("partition",
> +				    "invalid disk label magic 0x%x on partition %d\n"
> +				    label.magic, p.index);
> +		      continue;
> +		    }

Isn't a comma missing after the second string?  I am surprised that
this compiles.  Or am I missing something?

--
Marco

Re: [PATCH] skip over invalid BSD partitions

[Felix Zielcke]

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

Am Dienstag, den 05.08.2008, 15:10 +0200 schrieb Marco Gerards:
> Felix Zielcke <fzielcke87@googlemail.com> writes:
> 
> > 2008-08-05  Felix Zielcke  <fzielcke@z-51.de>
> >
> > 	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
> > 	magic or if there's no space left, use grub_dprintf to issue a warning.
> 
> Looks fine.
Hurray.
> 
> > +		    {
> > +		      grub_dprintf ("partition",
> > +				    "invalid disk label magic 0x%x on partition %d\n"
> > +				    label.magic, p.index);
> > +		      continue;
> > +		    }
> 
> Isn't a comma missing after the second string?  I am surprised that
> this compiles.  Or am I missing something?
> 

Bah, I noticed the 2 missing spaces and then missed that one out :(
Attached.

[-- Attachment #2: skip_invalid_bsd_parts.diff --]
[-- Type: text/x-patch, Size: 1136 bytes --]

Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(Revision 1770)
+++ partmap/pc.c	(Arbeitskopie)
@@ -160,9 +160,10 @@
 		{
 		  /* Check if the BSD label is within the DOS partition.  */
 		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "no space for disk label");
-
+		    {
+		      grub_dprintf ("partition", "no space for disk label\n");
+		      continue;
+		    }
 		  /* Read the BSD label.  */
 		  if (grub_disk_read (&raw,
 				      (p.start
@@ -175,10 +176,12 @@
 		  /* Check if it is valid.  */
 		  if (label.magic
 		      != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "invalid disk label magic 0x%x",
-				       label.magic);
-
+		    {
+		      grub_dprintf ("partition",
+				    "invalid disk label magic 0x%x on partition %d\n"
+				    label.magic, p.index);
+		      continue;
+		    }
 		  for (pcdata.bsd_part = 0;
 		       pcdata.bsd_part < grub_cpu_to_le16 (label.num_partitions);
 		       pcdata.bsd_part++)

Re: [PATCH] skip over invalid BSD partitions

[Felix Zielcke]

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

Am Dienstag, den 05.08.2008, 15:10 +0200 schrieb Marco Gerards:
> Felix Zielcke <fzielcke87@googlemail.com> writes:
> 
> > 2008-08-05  Felix Zielcke  <fzielcke@z-51.de>
> >
> > 	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
> > 	magic or if there's no space left, use grub_dprintf to issue a warning.
> 
> Looks fine.
Hurray.

> > +		    {
> > +		      grub_dprintf ("partition",
> > +				    "invalid disk label magic 0x%x on partition %d\n"
> > +				    label.magic, p.index);
> > +		      continue;
> > +		    }
> 
> Isn't a comma missing after the second string?  I am surprised that
> this compiles.  Or am I missing something?
> 
You're right, shame on me I notice to missing spaces but not a missing
comma.
Attached.

[-- Attachment #2: skip_invalid_bsd_parts.diff --]
[-- Type: text/x-patch, Size: 1137 bytes --]

Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(Revision 1770)
+++ partmap/pc.c	(Arbeitskopie)
@@ -160,9 +160,10 @@
 		{
 		  /* Check if the BSD label is within the DOS partition.  */
 		  if (p.len <= GRUB_PC_PARTITION_BSD_LABEL_SECTOR)
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "no space for disk label");
-
+		    {
+		      grub_dprintf ("partition", "no space for disk label\n");
+		      continue;
+		    }
 		  /* Read the BSD label.  */
 		  if (grub_disk_read (&raw,
 				      (p.start
@@ -175,10 +176,12 @@
 		  /* Check if it is valid.  */
 		  if (label.magic
 		      != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
-		    return grub_error (GRUB_ERR_BAD_PART_TABLE,
-				       "invalid disk label magic 0x%x",
-				       label.magic);
-
+		    {
+		      grub_dprintf ("partition",
+				    "invalid disk label magic 0x%x on partition %d\n",
+				    label.magic, p.index);
+		      continue;
+		    }
 		  for (pcdata.bsd_part = 0;
 		       pcdata.bsd_part < grub_cpu_to_le16 (label.num_partitions);
 		       pcdata.bsd_part++)

Re: [PATCH] skip over invalid BSD partitions

[Marco Gerards]

Felix Zielcke <fzielcke@z-51.de> writes:

> Am Dienstag, den 05.08.2008, 15:10 +0200 schrieb Marco Gerards:
>> Felix Zielcke <fzielcke87@googlemail.com> writes:
>> 
>> > 2008-08-05  Felix Zielcke  <fzielcke@z-51.de>
>> >
>> > 	* partmap/pc.c (pc_partition_map_iterate): Do not abort on an invalid BSD
>> > 	magic or if there's no space left, use grub_dprintf to issue a warning.
>> 
>> Looks fine.
> Hurray.
>
>> > +		    {
>> > +		      grub_dprintf ("partition",
>> > +				    "invalid disk label magic 0x%x on partition %d\n"
>> > +				    label.magic, p.index);
>> > +		      continue;
>> > +		    }
>> 
>> Isn't a comma missing after the second string?  I am surprised that
>> this compiles.  Or am I missing something?
>> 
> You're right, shame on me I notice to missing spaces but not a missing
> comma.
> Attached.

Seems fine to me :-)

--
Marco