Re: iterate return values

Re: iterate return values

[Hollis Blanchard]

This one took waaay too long to track down. :( It is very difficult to
debug when the problem is that grub_errno has a stale value.

This fixes my partition map problems, and has been tested both with
Apple and DOS partition maps.

-Hollis

Index: ChangeLog
===================================================================
RCS file: /cvsroot/grub/grub2/ChangeLog,v
retrieving revision 1.90
diff -u -p -r1.90 ChangeLog
--- ChangeLog	1 Feb 2005 21:53:34 -0000	1.90
+++ ChangeLog	4 Feb 2005 06:34:08 -0000
@@ -1,3 +1,13 @@
+2005-02-03  Hollis Blanchard  <hollis@penguinppc.org>
+
+	* kern/partition.c (grub_partition_probe): Clear `grub_errno' and
+	return 0 if `grub_errno' is GRUB_ERR_BAD_PART_TABLE. 
+	(part_map_iterate): Clear `grub_errno' and return 0 if
+	`partmap->iterate' returns GRUB_ERR_BAD_PART_TABLE. 
+	* partmap/amiga.c (amiga_partition_map_iterate): Return
+	GRUB_ERR_BAD_PART_TABLE if no partition map magic is found.
+	* partmap/apple.c (apple_partition_map_iterate): Likewise.
+
 2005-02-01  Guillem Jover  <guillem@hadrons.org>
 
 	* loader/i386/pc/multiboot_normal.c (GRUB_MOD_INIT): Fix module
Index: kern/partition.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/partition.c,v
retrieving revision 1.1
diff -u -p -r1.1 partition.c
--- kern/partition.c	4 Dec 2004 18:45:45 -0000	1.1
+++ kern/partition.c	4 Feb 2005 06:34:11 -0000
@@ -56,20 +56,28 @@ grub_partition_t
 grub_partition_probe (struct grub_disk *disk, const char *str)
 {
   grub_partition_t part;
-  
+
   auto int part_map_probe (const grub_partition_map_t partmap);
-  
+
   int part_map_probe (const grub_partition_map_t partmap)
     {
       part = partmap->probe (disk, str);
       if (part)
 	return 1;
-      return 0;
+
+      if (grub_errno == GRUB_ERR_BAD_PART_TABLE)
+	{
+	  /* Continue to next partition map type.  */
+	  grub_errno = GRUB_ERR_NONE;
+	  return 0;
+	}
+
+      return 1;
     }
 
   /* Use the first partition map type found.  */
   grub_partition_map_iterate (part_map_probe);
-  
+
   return part;
 }
 
@@ -78,12 +86,21 @@ grub_partition_iterate (struct grub_disk
 			int (*hook) (const grub_partition_t partition))
 {
   auto int part_map_iterate (const grub_partition_map_t partmap);
-  
+
   int part_map_iterate (const grub_partition_map_t partmap)
     {
-      return partmap->iterate (disk, hook);
+      grub_err_t err = partmap->iterate (disk, hook);
+
+      if (err == GRUB_ERR_BAD_PART_TABLE)
+	{
+	  /* Continue to next partition map type.  */
+	  grub_errno = GRUB_ERR_NONE;
+	  return 0;
+	}
+
+      return 1;
     }
-  
+
   grub_partition_map_iterate (part_map_iterate);
   return grub_errno;
 }
Index: partmap/amiga.c
===================================================================
RCS file: /cvsroot/grub/grub2/partmap/amiga.c,v
retrieving revision 1.1
diff -u -p -r1.1 amiga.c
--- partmap/amiga.c	4 Dec 2004 18:45:45 -0000	1.1
+++ partmap/amiga.c	4 Feb 2005 06:34:11 -0000
@@ -102,6 +102,10 @@ amiga_partition_map_iterate (grub_disk_t
 	  break;
 	}
     }
+
+  if (next == -1)
+    return grub_error (GRUB_ERR_BAD_PART_TABLE,
+		       "Amiga partition map not found.");
   
   /* The end of the partition list is marked using "-1".  */
   while (next != -1)
Index: partmap/apple.c
===================================================================
RCS file: /cvsroot/grub/grub2/partmap/apple.c,v
retrieving revision 1.1
diff -u -p -r1.1 apple.c
--- partmap/apple.c	4 Dec 2004 18:45:45 -0000	1.1
+++ partmap/apple.c	4 Feb 2005 06:34:11 -0000
@@ -108,7 +108,7 @@ apple_partition_map_iterate (grub_disk_t
   raw.partition = 0;
 
   part.partmap = &grub_apple_partition_map;
-  
+
   for (;;)
     {
       if (grub_disk_read (&raw, pos / GRUB_DISK_SECTOR_SIZE,
@@ -134,6 +134,10 @@ apple_partition_map_iterate (grub_disk_t
       partno++;
     }
 
+  if ((pos / GRUB_DISK_SECTOR_SIZE) == 0)
+    return grub_error (GRUB_ERR_BAD_PART_TABLE,
+		       "Apple partition map not found.");
+
   return 0;
 }
 
@@ -178,7 +182,6 @@ apple_partition_map_probe (grub_disk_t d
  fail:
   grub_free (p);
   return 0;
-
 }
 
 

Re: iterate return values

[Yoshinori K. Okuji]

On Friday 04 February 2005 07:11, Hollis Blanchard wrote:
> This one took waaay too long to track down. :( It is very difficult
> to debug when the problem is that grub_errno has a stale value.

It might be a good idea that grub_error can output an error immediately 
when debug mode is enabled.

Okuji

Re: iterate return values

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> This one took waaay too long to track down. :( It is very difficult to
> debug when the problem is that grub_errno has a stale value.
>
> This fixes my partition map problems, and has been tested both with
> Apple and DOS partition maps.

That's nice!  There are a few comments.  If you fixed those minor
issues, feel free to commit the patch.

>
> -Hollis
>
> Index: ChangeLog
> ===================================================================
> RCS file: /cvsroot/grub/grub2/ChangeLog,v
> retrieving revision 1.90
> diff -u -p -r1.90 ChangeLog
> --- ChangeLog	1 Feb 2005 21:53:34 -0000	1.90
> +++ ChangeLog	4 Feb 2005 06:34:08 -0000
> @@ -1,3 +1,13 @@
> +2005-02-03  Hollis Blanchard  <hollis@penguinppc.org>

The changelog is fine for me, but please don't send it in as a patch.

> -  
> +
>    auto int part_map_probe (const grub_partition_map_t partmap);
> -  
> +

Please watch out for changes like these.

> +      if (err == GRUB_ERR_BAD_PART_TABLE)
> +	{
> +	  /* Continue to next partition map type.  */
> +	  grub_errno = GRUB_ERR_NONE;
> +	  return 0;
> +	}

This looks like a tab, please don't use tabs.  I hope you can fix it
if that is the case and the same for the rest of the patch, if the
same happened there.

Thanks,
Marco

Re: iterate return values

[Yoshinori K. Okuji]

On Friday 04 February 2005 21:04, Marco Gerards wrote:
> The changelog is fine for me, but please don't send it in as a patch.

For me, it is fine to include ChangeLog entries in a patch. What is 
wrong?

> This looks like a tab, please don't use tabs.  I hope you can fix it
> if that is the case and the same for the rest of the patch, if the
> same happened there.

Hmmh, I think I use tabs, too. Should I stop using tabs inside GRUB? In 
the past, tabs were prefered, because using only spaces enlarged the 
source code and the network bandwidth was extremely narrow. Nowadays, 
these reasons are not effective any longer.

Okuji

Re: iterate return values

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Friday 04 February 2005 21:04, Marco Gerards wrote:
>> The changelog is fine for me, but please don't send it in as a patch.
>
> For me, it is fine to include ChangeLog entries in a patch. What is 
> wrong?

When you apply the patch it can not be applied when the changelog was
changed by another commit.

>> This looks like a tab, please don't use tabs.  I hope you can fix it
>> if that is the case and the same for the rest of the patch, if the
>> same happened there.
>
> Hmmh, I think I use tabs, too. Should I stop using tabs inside GRUB? In 
> the past, tabs were prefered, because using only spaces enlarged the 
> source code and the network bandwidth was extremely narrow. Nowadays, 
> these reasons are not effective any longer.

Perhaps I am just confused.  I was under the impression the GCS forces
you to use spaces, but I can't find anything about it.  And emacs
creates tabs too when indenting.  So it seems I was just very wrong.

Sorry for the confusion.

Thanks,
Marco

Re: iterate return values

[Yoshinori K. Okuji]

On Friday 04 February 2005 22:21, Marco Gerards wrote:
> > For me, it is fine to include ChangeLog entries in a patch. What is
> > wrong?
>
> When you apply the patch it can not be applied when the changelog was
> changed by another commit.

In that case, you just get it from ChangeLog.rej, no?

I think our opinions are different because I rarely use the command 
"patch" to apply patches for development. I usually apply patches by 
hand, since I always want to modify patches more or less before 
applying them. This might be solved if I start complaining a lot in 
order to let contributors modify patches as I wish, but I think most of 
them would be merely scared and rush way...

> Perhaps I am just confused.  I was under the impression the GCS
> forces you to use spaces, but I can't find anything about it.  And
> emacs creates tabs too when indenting.  So it seems I was just very
> wrong.

But you are right actually, since tabs are mostly useless and rather 
confusing.

Okuji

Re: iterate return values

[Hollis Blanchard]

This patch fixed it for me, tested on Apple and DOS partition maps.

(The #if 0 stuff is an example of what I'm thinking about for
grub_debug_printf, btw, I will remove it from this patch before checking
in.)

Is this OK?

-Hollis

2005-01-23  Hollis Blanchard  <hollis@penguinppc.org>

	* include/grub/partition.h (grub_partition_map): Remove trailing
	whitespace.  Add `present' function pointer.
	(grub_partition_map_iterate): Add `disk' argument.
	* kern/partition.c (grub_partition_map_iterate): Likewise.  Update all
	callers.  Call `hook' only if `present' returns true.
	* partmap/amiga.c (amiga_partition_map_present): New function.
	(grub_amiga_partition_map): Initialize `present'.
	* partmap/apple.c (apple_partition_map_present): New function.
	(grub_apple_partition_map): Initialize `present'.
	* partmap/pc.c (pc_partition_map_present): New function.
	(grub_pc_partition_map): Initialize `present'.


Index: include/grub/partition.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/partition.h,v
retrieving revision 1.1
diff -u -p -r1.1 partition.h
--- include/grub/partition.h	4 Dec 2004 18:45:45 -0000	1.1
+++ include/grub/partition.h	23 Jan 2005 18:37:45 -0000
@@ -31,18 +31,21 @@ struct grub_partition_map
 {
   /* The name of the partition map type.  */
   const char *name;
-  
+
+  /* Is this partition map type present on `disk'? */
+  grub_err_t (*present) (struct grub_disk *disk);
+
   /* Call HOOK with each partition, until HOOK returns non-zero.  */
   grub_err_t (*iterate) (struct grub_disk *disk,
 			 int (*hook) (const grub_partition_t partition));
-  
+
   /* Return the partition named STR on the disk DISK.  */
   grub_partition_t (*probe) (struct grub_disk *disk,
 			     const char *str);
-  
+
   /* Return the name of the partition PARTITION.  */
   char *(*get_name) (const grub_partition_t partition);
-  
+
   /* The next partition map type.  */
   struct grub_partition_map *next;
 };
@@ -76,7 +79,8 @@ grub_err_t EXPORT_FUNC(grub_partition_it
 						int (*hook) (const grub_partition_t partition));
 char *EXPORT_FUNC(grub_partition_get_name) (const grub_partition_t partition);
 
-void EXPORT_FUNC(grub_partition_map_iterate) (int (*hook) (const grub_partition_map_t partmap));
+void EXPORT_FUNC(grub_partition_map_iterate) (struct grub_disk *disk,
+					      int (*hook) (const grub_partition_map_t partmap));
 					      
 void EXPORT_FUNC(grub_partition_map_register) (grub_partition_map_t partmap);
 
Index: kern/partition.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/partition.c,v
retrieving revision 1.1
diff -u -p -r1.1 partition.c
--- kern/partition.c	4 Dec 2004 18:45:45 -0000	1.1
+++ kern/partition.c	23 Jan 2005 18:37:46 -0000
@@ -43,13 +43,27 @@ grub_partition_map_unregister (grub_part
 }
 
 void
-grub_partition_map_iterate (int (*hook) (const grub_partition_map_t partmap))
+grub_partition_map_iterate (struct grub_disk *disk,
+			    int (*hook) (const grub_partition_map_t partmap))
 {
   grub_partition_map_t p;
 
   for (p = grub_partition_map_list; p; p = p->next)
-    if (hook (p))
+  {
+    int present;
+
+    grub_errno = 0;
+    present = p->present (disk);
+
+#if 0
+    grub_printf ("%s: %s returned %d\n", __FUNCTION__, p->name, present);
+    if (! present)
+      grub_print_error ();
+#endif
+
+    if (present && hook (p))
       break;
+  }
 }
 
 grub_partition_t
@@ -68,7 +82,7 @@ grub_partition_probe (struct grub_disk *
     }
 
   /* Use the first partition map type found.  */
-  grub_partition_map_iterate (part_map_probe);
+  grub_partition_map_iterate (disk, part_map_probe);
   
   return part;
 }
@@ -84,7 +98,7 @@ grub_partition_iterate (struct grub_disk
       return partmap->iterate (disk, hook);
     }
   
-  grub_partition_map_iterate (part_map_iterate);
+  grub_partition_map_iterate (disk, part_map_iterate);
   return grub_errno;
 }
 
Index: partmap/amiga.c
===================================================================
RCS file: /cvsroot/grub/grub2/partmap/amiga.c,v
retrieving revision 1.1
diff -u -p -r1.1 amiga.c
--- partmap/amiga.c	4 Dec 2004 18:45:45 -0000	1.1
+++ partmap/amiga.c	23 Jan 2005 18:37:46 -0000
@@ -72,6 +72,35 @@ static struct grub_partition_map grub_am
 static grub_dl_t my_mod;
 #endif
 \f
+
+static grub_err_t
+amiga_partition_map_present (grub_disk_t disk)
+{
+  struct grub_amiga_rdsk rdsk;
+  struct grub_disk raw;
+  int pos;
+
+  /* Enforce raw disk access.  */
+  raw = *disk;
+  raw.partition = 0;
+
+  /* The RDSK block is one of the first 15 blocks.  */
+  for (pos = 0; pos < 15; pos++)
+    {
+      /* Read the RDSK block which is a descriptor for the entire disk.  */
+      if (grub_disk_read (&raw, pos, 0, sizeof (rdsk),  (char *) &rdsk))
+	goto finish;
+      
+      if (!grub_strcmp (rdsk.magic, "RDSK"))
+	break;
+    }
+
+  grub_error (GRUB_ERR_BAD_PART_TABLE, "no Amiga magic");
+
+finish:
+  return ! grub_errno;
+}
+
 static grub_err_t
 amiga_partition_map_iterate (grub_disk_t disk,
 			     int (*hook) (const grub_partition_t partition))
@@ -198,6 +227,7 @@ amiga_partition_map_get_name (const grub
 static struct grub_partition_map grub_amiga_partition_map =
   {
     .name = "amiga_partition_map",
+    .present = amiga_partition_map_present,
     .iterate = amiga_partition_map_iterate,
     .probe = amiga_partition_map_probe,
     .get_name = amiga_partition_map_get_name
Index: partmap/apple.c
===================================================================
RCS file: /cvsroot/grub/grub2/partmap/apple.c,v
retrieving revision 1.1
diff -u -p -r1.1 apple.c
--- partmap/apple.c	4 Dec 2004 18:45:45 -0000	1.1
+++ partmap/apple.c	23 Jan 2005 18:37:46 -0000
@@ -94,6 +94,27 @@ static grub_dl_t my_mod;
 \f
 
 static grub_err_t
+apple_partition_map_present (grub_disk_t disk)
+{
+  struct grub_apple_part apart;
+  struct grub_disk raw;
+
+  /* Enforce raw disk access.  */
+  raw = *disk;
+  raw.partition = 0;
+
+  if (grub_disk_read (&raw, 1, 0, sizeof (struct grub_apple_part),
+		      (char *) &apart))
+    goto finish;
+
+  if (apart.magic != GRUB_APPLE_PART_MAGIC)
+    grub_error (GRUB_ERR_BAD_PART_TABLE, "missing Apple magic");
+
+finish:
+  return ! grub_errno;
+}
+
+static grub_err_t
 apple_partition_map_iterate (grub_disk_t disk,
 			int (*hook) (const grub_partition_t partition))
 {
@@ -200,6 +221,7 @@ apple_partition_map_get_name (const grub
 static struct grub_partition_map grub_apple_partition_map =
   {
     .name = "apple_partition_map",
+    .present = apple_partition_map_present,
     .iterate = apple_partition_map_iterate,
     .probe = apple_partition_map_probe,
     .get_name = apple_partition_map_get_name
Index: partmap/pc.c
===================================================================
RCS file: /cvsroot/grub/grub2/partmap/pc.c,v
retrieving revision 1.1
diff -u -p -r1.1 pc.c
--- partmap/pc.c	4 Dec 2004 18:45:45 -0000	1.1
+++ partmap/pc.c	23 Jan 2005 18:37:46 -0000
@@ -92,6 +92,28 @@ grub_partition_parse (const char *str)
 }
 
 static grub_err_t
+pc_partition_map_present (grub_disk_t disk)
+{
+  struct grub_disk raw;
+  struct grub_pc_partition_mbr mbr;
+
+  /* Enforce raw disk access.  */
+  raw = *disk;
+  raw.partition = 0;
+
+  /* Read the MBR.  */
+  if (grub_disk_read (&raw, 0, 0, sizeof (mbr), (char *) &mbr))
+    goto finish;
+
+  /* Check if it is valid.  */
+  if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
+    grub_error (GRUB_ERR_BAD_PART_TABLE, "missing DOS signature");
+
+finish:
+  return ! grub_errno;
+}
+
+static grub_err_t
 pc_partition_map_iterate (grub_disk_t disk,
 			  int (*hook) (const grub_partition_t partition))
 {
@@ -283,6 +305,7 @@ pc_partition_map_get_name (const grub_pa
 static struct grub_partition_map grub_pc_partition_map =
   {
     .name = "pc_partition_map",
+    .present = pc_partition_map_present,
     .iterate = pc_partition_map_iterate,
     .probe = pc_partition_map_probe,
     .get_name = pc_partition_map_get_name

Re: iterate return values

[Yoshinori K. Okuji]

On Sunday 23 January 2005 19:23, Hollis Blanchard wrote:
> This patch fixed it for me, tested on Apple and DOS partition maps.

I vote for not adding an additional function. I think it is simpler to 
check an error rather than calling "present" functions. I don't want to 
bloat the API.

Okuji

Re: iterate return values

[Hollis Blanchard]

On Jan 23, 2005, at 1:19 PM, Yoshinori K. Okuji wrote:

> On Sunday 23 January 2005 19:23, Hollis Blanchard wrote:
>> This patch fixed it for me, tested on Apple and DOS partition maps.
>
> I vote for not adding an additional function. I think it is simpler to
> check an error rather than calling "present" functions. I don't want to
> bloat the API.

I think "is this partition type present" is a very reasonable API to 
support...

What is the alternative? How would you distinguish between an iterate 
error (like "wrong partition map type") and a hook returning 1?

-Hollis

Re: iterate return values

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> On Jan 23, 2005, at 1:19 PM, Yoshinori K. Okuji wrote:
>
>> On Sunday 23 January 2005 19:23, Hollis Blanchard wrote:
>>> This patch fixed it for me, tested on Apple and DOS partition maps.
>>
>> I vote for not adding an additional function. I think it is simpler to
>> check an error rather than calling "present" functions. I don't want to
>> bloat the API.
>
> I think "is this partition type present" is a very reasonable API to
> support...
>
> What is the alternative? How would you distinguish between an iterate
> error (like "wrong partition map type") and a hook returning 1?

You could check for the specific error (GRUB_ERR_INCORRECT_PART or
so).

--
Marco

Re: iterate return values

[Yoshinori K. Okuji]

On Sunday 23 January 2005 20:53, Hollis Blanchard wrote:
> I think "is this partition type present" is a very reasonable API to
> support...

Then, the filesystem also must have a function for "if this file is 
present". This is overkill.

> What is the alternative? How would you distinguish between an iterate
> error (like "wrong partition map type") and a hook returning 1?

So I said that iterate functions should not return non-zero on errors. 
They should return zero instead. Then, you can check if it was because 
of an error or not.

Okuji

iterate return values

[Hollis Blanchard]

I'm having trouble seeing my Apple-partitioned disk with pc.mod loaded. 
The trouble seems to be a misunderstanding about how iterate methods 
should return.

When a hook is passed to grub_partition_iterate, it is applied to 
partmap->iterate. However, the partition map's iterate functions can 
return non-zero in case of error (such as "not a PC partition map"). 
Non-zero is then propagated back to grub_partition_map_iterate, and the 
iteration stops. In that case we should clearly keep going to try 
another partition map type.

However, the other case is that the hook itself returned non-zero, 
indicating it wishes to stop the iteration. As far as I can see there 
is no way to distinguish this from the above case. Or are there 
actually any hooks that want to stop iteration? If not, the test in 
grub_partition_map_iterate can go. (But that doesn't seem to fix my 
problem either... sigh.)

-Hollis

Re: iterate return values

[Yoshinori K. Okuji]

On Saturday 22 January 2005 07:57, Hollis Blanchard wrote:
> When a hook is passed to grub_partition_iterate, it is applied to
> partmap->iterate. However, the partition map's iterate functions can
> return non-zero in case of error (such as "not a PC partition map").
> Non-zero is then propagated back to grub_partition_map_iterate, and
> the iteration stops. In that case we should clearly keep going to try
> another partition map type.

It's a big problem. I think grub_partition_iterate should not return 
non-zero when it fails.

Okuji

Re: iterate return values

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> I'm having trouble seeing my Apple-partitioned disk with pc.mod
> loaded. The trouble seems to be a misunderstanding about how iterate
> methods should return.
>
> When a hook is passed to grub_partition_iterate, it is applied to
>  partmap->iterate. However, the partition map's iterate functions can
> return non-zero in case of error (such as "not a PC partition
> map"). Non-zero is then propagated back to grub_partition_map_iterate,
> and the iteration stops. In that case we should clearly keep going to
> try another partition map type.

Right.  Normally iterate functions in GRUB return 1 when the iteration
should stop.  For the iterate function of partition maps this is not
the case, but this is how grub_partition_iterate tries to work.  You
can even see the return type of grub_err_t is directory returned as
int, this is a bug.

> However, the other case is that the hook itself returned non-zero,
> indicating it wishes to stop the iteration. As far as I can see there
> is no way to distinguish this from the above case. Or are there
> actually any hooks that want to stop iteration? If not, the test in
> grub_partition_map_iterate can go. (But that doesn't seem to fix my
> problem either... sigh.)

Right.  I think grub_partition_iterate and the iterate functions for
the partition maps should be changed so they work like an iterate
function and return 0 or 1, the value returned by the hook.

I think it would be easier to add a function to the partition maps to
test if the disk has this kind of partition map.  This will make
things a lot easier and cleaner.

What do you think?

Thanks,
Marco