[PATCH] simplify grub_raid_array

[PATCH] simplify grub_raid_array

[Robert Millan]

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


Unless I missed something, it seems that grub_raid_array contains redundant
information (`name' is already present via `disk->name').  I propose to
simplify it this way.

-- 
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: simplify_grub_raid_array.diff --]
[-- Type: text/x-diff, Size: 3618 bytes --]

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/raid.c ./disk/raid.c
--- ../grub2/disk/raid.c	2007-12-30 09:52:03.000000000 +0100
+++ ./disk/raid.c	2008-02-06 17:35:24.000000000 +0100
@@ -146,7 +146,7 @@ grub_raid_read (grub_disk_t disk, grub_d
 	  {
 	    grub_uint32_t i;
 
-	    err = grub_disk_read (array->device[disknr].disk, read_sector, 0,
+	    err = grub_disk_read (array->device[disknr], read_sector, 0,
 				  read_size << GRUB_DISK_SECTOR_BITS, buf);
 	    if (err)
 	      break;
@@ -189,9 +189,9 @@ grub_raid_read (grub_disk_t disk, grub_d
 	
 	for (i = 0; i < array->total_devs; i++)
 	  {
-	    if (array->device[i].disk)
+	    if (array->device[i])
 	      {
-		err = grub_disk_read (array->device[i].disk, sector, 0,
+		err = grub_disk_read (array->device[i], sector, 0,
 				      size << GRUB_DISK_SECTOR_BITS, buf);
 
 		if (!err)
@@ -234,8 +234,8 @@ grub_raid_read (grub_disk_t disk, grub_d
 	  {
 	    grub_uint32_t i;
 
-	    if (array->device[disknr].disk)
-	      err = grub_disk_read (array->device[disknr].disk, read_sector, 0,
+	    if (array->device[disknr])
+	      err = grub_disk_read (array->device[disknr], read_sector, 0,
 				    read_size << GRUB_DISK_SECTOR_BITS, buf);
 
 	    /* If an error occurs when we already have an degraded
@@ -243,7 +243,7 @@ grub_raid_read (grub_disk_t disk, grub_d
 	    if (err && ((array->total_devs - 1) == array->nr_devs))
 	      break;
 	    
-	    if (err || ! array->device[disknr].disk)
+	    if (err || ! array->device[disknr])
 	      {
 		/* Either an error occured or the disk is not
 		   available.  We have to compute this block from the
@@ -260,7 +260,7 @@ grub_raid_read (grub_disk_t disk, grub_d
 
 		    if (j != (unsigned int) disknr)
 		      {
-			err = grub_disk_read (array->device[j].disk, read_sector,
+			err = grub_disk_read (array->device[j], read_sector,
 					      0, buf_size, buf2);
 			if (err)
 			  return err;
@@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
 	  return 0;
 	}
   
-      if (array->device[sb.this_disk.number].name != 0)
+      if (array->device[sb.this_disk.number]->name != 0)
 	{
 	  /* We found multiple devices with the same number. Again,
 	     this shouldn't happen.*/
@@ -491,14 +491,10 @@ grub_raid_scan_device (const char *name)
     }
 
   /* Add the device to the array. */
-  array->device[sb.this_disk.number].name = grub_strdup (name);
-  array->device[sb.this_disk.number].disk = grub_disk_open (name);
+  array->device[sb.this_disk.number] = grub_disk_open (name);
   
-  if (! array->device[sb.this_disk.number].name
-      || ! array->device[sb.this_disk.number].disk)
+  if (! array->device[sb.this_disk.number])
     {
-      grub_free (array->device[sb.this_disk.number].name);
-
       /* Remove array from the list if we have just added it. */
       if (array->nr_devs == 0)
 	{
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/include/grub/raid.h ./include/grub/raid.h
--- ../grub2/include/grub/raid.h	2007-07-22 01:32:22.000000000 +0200
+++ ./include/grub/raid.h	2008-02-06 17:34:37.000000000 +0100
@@ -37,11 +37,7 @@ struct grub_raid_array
   char *name;              /* That will be "md<number>". */
   grub_uint64_t disk_size; /* Size of an individual disk, in 512 byte
 			      sectors. */
-  struct
-  {
-    char *name;            /* Name of the device */
-    grub_disk_t disk;      /* The device itself. */
-  } device[32];            /* Array of total_devs devices. */          
+  grub_disk_t device[32];  /* Array of total_devs devices. */          
   struct grub_raid_array *next;
 };
 

Re: [PATCH] simplify grub_raid_array

[Jeroen Dekkers]

At Wed, 6 Feb 2008 17:45:07 +0100,
Robert Millan wrote:
> Unless I missed something, it seems that grub_raid_array contains redundant
> information (`name' is already present via `disk->name').  I propose to
> simplify it this way.

No idea why, I don't have the time to look at the actual code, but 
 
> @@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
>  	  return 0;
>  	}
>    
> -      if (array->device[sb.this_disk.number].name != 0)
> +      if (array->device[sb.this_disk.number]->name != 0)
>  	{
>  	  /* We found multiple devices with the same number. Again,
>  	     this shouldn't happen.*/

looks suspicious to me. Is that really doing what it is meant to do?


Regards,

Jeroen Dekkers

Re: [PATCH] simplify grub_raid_array

[Robert Millan]

On Wed, Feb 06, 2008 at 11:43:39PM +0100, Jeroen Dekkers wrote:
> At Wed, 6 Feb 2008 17:45:07 +0100,
> Robert Millan wrote:
> > Unless I missed something, it seems that grub_raid_array contains redundant
> > information (`name' is already present via `disk->name').  I propose to
> > simplify it this way.
> 
> No idea why, I don't have the time to look at the actual code, but 
>  
> > @@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
> >  	  return 0;
> >  	}
> >    
> > -      if (array->device[sb.this_disk.number].name != 0)
> > +      if (array->device[sb.this_disk.number]->name != 0)
> >  	{
> >  	  /* We found multiple devices with the same number. Again,
> >  	     this shouldn't happen.*/
> 
> looks suspicious to me. Is that really doing what it is meant to do?

Yes.  `array->device[sb.this_disk.number]' used to be:

  struct { char *name; grub_disk_t disk; }

and it becomes grub_disk_t, which is:

  struct { char *name; other stuff; } *

this is tested in runtime.  My concern was more about whether both `name'
instances could have different meaning or so.

-- 
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] simplify grub_raid_array

[Jeroen Dekkers]

At Wed, 6 Feb 2008 23:59:54 +0100,
Robert Millan wrote:
> 
> On Wed, Feb 06, 2008 at 11:43:39PM +0100, Jeroen Dekkers wrote:
> > At Wed, 6 Feb 2008 17:45:07 +0100,
> > Robert Millan wrote:
> > > Unless I missed something, it seems that grub_raid_array contains redundant
> > > information (`name' is already present via `disk->name').  I propose to
> > > simplify it this way.
> > 
> > No idea why, I don't have the time to look at the actual code, but 
> >  
> > > @@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
> > >  	  return 0;
> > >  	}
> > >    
> > > -      if (array->device[sb.this_disk.number].name != 0)
> > > +      if (array->device[sb.this_disk.number]->name != 0)
> > >  	{
> > >  	  /* We found multiple devices with the same number. Again,
> > >  	     this shouldn't happen.*/
> > 
> > looks suspicious to me. Is that really doing what it is meant to do?
> 
> Yes.  

No, it doesn't. The meaning of the check .name != 0 is whether a
device with that number already exists in the array. If .name is 0, it
doesn't exist yet, because else it would've been assigned
something. What you're now doing is wrong because if there isn't a
device in the array then array->device[sb.this_disk.number] is 0. I
guess the only reason this actually works is because the first page of
memory (the one at address 0) happens to be zero by luck.


Jeroen Dekkers

Re: [PATCH] simplify grub_raid_array

[Robert Millan]

On Fri, Feb 08, 2008 at 11:56:07PM +0100, Jeroen Dekkers wrote:
> At Wed, 6 Feb 2008 23:59:54 +0100,
> Robert Millan wrote:
> > 
> > On Wed, Feb 06, 2008 at 11:43:39PM +0100, Jeroen Dekkers wrote:
> > > At Wed, 6 Feb 2008 17:45:07 +0100,
> > > Robert Millan wrote:
> > > > Unless I missed something, it seems that grub_raid_array contains redundant
> > > > information (`name' is already present via `disk->name').  I propose to
> > > > simplify it this way.
> > > 
> > > No idea why, I don't have the time to look at the actual code, but 
> > >  
> > > > @@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
> > > >  	  return 0;
> > > >  	}
> > > >    
> > > > -      if (array->device[sb.this_disk.number].name != 0)
> > > > +      if (array->device[sb.this_disk.number]->name != 0)
> > > >  	{
> > > >  	  /* We found multiple devices with the same number. Again,
> > > >  	     this shouldn't happen.*/
> > > 
> > > looks suspicious to me. Is that really doing what it is meant to do?
> > 
> > Yes.  
> 
> No, it doesn't. The meaning of the check .name != 0 is whether a
> device with that number already exists in the array. If .name is 0, it
> doesn't exist yet, because else it would've been assigned
> something. What you're now doing is wrong because if there isn't a
> device in the array then array->device[sb.this_disk.number] is 0. I
> guess the only reason this actually works is because the first page of
> memory (the one at address 0) happens to be zero by luck.

Yep.  I just fixed this a while ago (Sam caught that in a segfault).

Does it look good now?

-- 
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] simplify grub_raid_array

[Robert Millan]

On Wed, Feb 06, 2008 at 05:45:07PM +0100, Robert Millan wrote:
> 
> Unless I missed something, it seems that grub_raid_array contains redundant
> information (`name' is already present via `disk->name').  I propose to
> simplify it this way.

Committed.

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