[PATCH] (ata.mod) avoid passing grub_errno to upper layer

[PATCH] (ata.mod) avoid passing grub_errno to upper layer

[Robert Millan]

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


Hi,

When an error is detected by ata.mod during drive scan, it will pass it to
the upper layer.  This results in GRUB aborting when trying to enter normal
mode, even if the error is not critical (e.g. affects a drive not used during
boot).

There are a number of places in ata.mod where these errors could be handled,
so I'm not sure if my proposed change would be the best approach.  Some comment
would be appreciated.

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

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

2008-11-22  Robert Millan  <rmh@aybabtu.com>

	* disk/ata.c (grub_ata_device_initialize): Avoid passing grub_errno
	conditions to the upper layer unless they're critical.

Index: disk/ata.c
===================================================================
--- disk/ata.c	(revision 1926)
+++ disk/ata.c	(working copy)
@@ -421,19 +421,13 @@ grub_ata_device_initialize (int port, in
   grub_ata_regset (dev, GRUB_ATA_REG_SECTORS, 0x5A);  
   grub_ata_wait ();
   if (grub_ata_regget (dev, GRUB_ATA_REG_SECTORS) != 0x5A)
-    {
-      grub_free(dev);
-      return 0;
-    }
+    goto fail;
 
   /* Detect if the device is present by issuing a EXECUTE
      DEVICE DIAGNOSTICS command.  */
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, dev->device << 4);
   if (grub_ata_cmd (dev, GRUB_ATA_CMD_EXEC_DEV_DIAGNOSTICS))
-    {
-      grub_free (dev);
-      return grub_errno;
-    }
+    goto fail;
   grub_ata_wait ();
 
   grub_dprintf ("ata", "Registers: %x %x %x %x\n",
@@ -460,23 +454,29 @@ grub_ata_device_initialize (int port, in
   else
     {
       grub_dprintf ("ata", "incorrect signature\n");
-      grub_free (dev);
-      return 0;
+      goto fail;
     }
 
 
   /* Use the IDENTIFY DEVICE command to query the device.  */
   if (grub_ata_identify (dev))
-    {
-      grub_free (dev);
-      return 0;
-    }
+    goto fail;
 
   /* Register the device.  */
   for (devp = &grub_ata_devices; *devp; devp = &(*devp)->next);
   *devp = dev;
 
   return 0;
+
+ fail:
+  grub_free (dev);
+
+  /* If there were errors it means we didn't register this device.  Since other
+     devices may be fine, we don't pass this to the upper layer.  */
+  if (grub_errno)
+    grub_print_error ();
+
+  return 0;
 }
 
 static int

Re: [PATCH] (ata.mod) avoid passing grub_errno to upper layer

[Yoshinori K. Okuji]

On Saturday 22 November 2008 16:35:09 Robert Millan wrote:
> When an error is detected by ata.mod during drive scan, it will pass it to
> the upper layer.  This results in GRUB aborting when trying to enter normal
> mode, even if the error is not critical (e.g. affects a drive not used
> during boot).
>
> There are a number of places in ata.mod where these errors could be
> handled, so I'm not sure if my proposed change would be the best approach. 
> Some comment would be appreciated.

This is one way, but I think the upper layer should be more robust against 
errors raised from modules. For example, we can unload a module, and clear 
GRUB_ERRNO, if the init function in this module return an error.

Regards,
Okuji

Re: [PATCH] (ata.mod) avoid passing grub_errno to upper layer

[Robert Millan]

On Tue, Nov 25, 2008 at 10:17:17PM +0100, Yoshinori K. Okuji wrote:
> On Saturday 22 November 2008 16:35:09 Robert Millan wrote:
> > When an error is detected by ata.mod during drive scan, it will pass it to
> > the upper layer.  This results in GRUB aborting when trying to enter normal
> > mode, even if the error is not critical (e.g. affects a drive not used
> > during boot).
> >
> > There are a number of places in ata.mod where these errors could be
> > handled, so I'm not sure if my proposed change would be the best approach. 
> > Some comment would be appreciated.
> 
> This is one way, but I think the upper layer should be more robust against 
> errors raised from modules. For example, we can unload a module, and clear 
> GRUB_ERRNO, if the init function in this module return an error.

But if the error is specific to a device unit, unloading the module would
result in all units of this device class being disabled, which is most likely
not what we want.

-- 
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] (ata.mod) avoid passing grub_errno to upper layer

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Tue, Nov 25, 2008 at 10:17:17PM +0100, Yoshinori K. Okuji wrote:
>> On Saturday 22 November 2008 16:35:09 Robert Millan wrote:
>>> When an error is detected by ata.mod during drive scan, it will pass it to
>>> the upper layer.  This results in GRUB aborting when trying to enter normal
>>> mode, even if the error is not critical (e.g. affects a drive not used
>>> during boot).
>>>
>>> There are a number of places in ata.mod where these errors could be
>>> handled, so I'm not sure if my proposed change would be the best approach. 
>>> Some comment would be appreciated.
>> This is one way, but I think the upper layer should be more robust against 
>> errors raised from modules. For example, we can unload a module, and clear 
>> GRUB_ERRNO, if the init function in this module return an error.
> 
> But if the error is specific to a device unit, unloading the module would
> result in all units of this device class being disabled, which is most likely
> not what we want.
> 

Perhaps this should then be handled on generic code performing the scan?

Just give unique error type and just ignore the error (or dump it to
screen) and continue to next device/operation.

Re: [PATCH] (ata.mod) avoid passing grub_errno to upper layer

[Robert Millan]

On Sat, Nov 29, 2008 at 12:01:54PM +0200, Vesa Jääskeläinen wrote:
> Robert Millan wrote:
> > On Tue, Nov 25, 2008 at 10:17:17PM +0100, Yoshinori K. Okuji wrote:
> >> On Saturday 22 November 2008 16:35:09 Robert Millan wrote:
> >>> When an error is detected by ata.mod during drive scan, it will pass it to
> >>> the upper layer.  This results in GRUB aborting when trying to enter normal
> >>> mode, even if the error is not critical (e.g. affects a drive not used
> >>> during boot).
> >>>
> >>> There are a number of places in ata.mod where these errors could be
> >>> handled, so I'm not sure if my proposed change would be the best approach. 
> >>> Some comment would be appreciated.
> >> This is one way, but I think the upper layer should be more robust against 
> >> errors raised from modules. For example, we can unload a module, and clear 
> >> GRUB_ERRNO, if the init function in this module return an error.
> > 
> > But if the error is specific to a device unit, unloading the module would
> > result in all units of this device class being disabled, which is most likely
> > not what we want.
> > 
> 
> Perhaps this should then be handled on generic code performing the scan?
> 
> Just give unique error type and just ignore the error (or dump it to
> screen) and continue to next device/operation.

What do you mean generic code?  It's all in ata.mod:

  - grub_ata_initialize () invokes grub_pci_iterate (grub_ata_pciinit)

  - grub_ata_pciinit () runs grub_ata_device_initialize () on a few
    prospective device units

  - grub_ata_device_initialize () might return with grub_errno != 0

or you mean in grub_ata_pciiinit() ?

-- 
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] (ata.mod) avoid passing grub_errno to upper layer

[Robert Millan]

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


Okay, I think I got what you mean.  So how about this one?

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

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

2008-11-29  Robert Millan  <rmh@aybabtu.com>

	* disk/ata.c (grub_ata_pciinit): Handle errors rised by
	grub_ata_device_initialize() calls.

Index: disk/ata.c
===================================================================
--- disk/ata.c	(revision 1929)
+++ disk/ata.c	(working copy)
@@ -542,6 +542,16 @@ grub_ata_pciinit (int bus, int device, i
 	{
 	  grub_ata_device_initialize (controller * 2 + i, 0, rega, regb);
 	  grub_ata_device_initialize (controller * 2 + i, 1, rega, regb);
+
+	  /* Most errors rised by grub_ata_device_initialize() are harmless.
+	     They just indicate this particular drive is not responding, most
+	     likely because it doesn't exist.  We might want to ignore specific
+	     error types here, instead of printing them.  */
+	  if (grub_errno)
+	    {
+	      grub_print_error ();
+	      grub_errno = GRUB_ERR_NONE;
+	    }
 	}
     }
 

Re: [PATCH] (ata.mod) avoid passing grub_errno to upper layer

[Robert Millan]

Committed (after repeating the same block for both calls, as Vesa suggested
on IRC).

On Sat, Nov 29, 2008 at 07:37:46PM +0100, Robert Millan wrote:
> 
> Okay, I think I got what you mean.  So how about this one?
> 
> -- 
> 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."

> 2008-11-29  Robert Millan  <rmh@aybabtu.com>
> 
> 	* disk/ata.c (grub_ata_pciinit): Handle errors rised by
> 	grub_ata_device_initialize() calls.
> 
> Index: disk/ata.c
> ===================================================================
> --- disk/ata.c	(revision 1929)
> +++ disk/ata.c	(working copy)
> @@ -542,6 +542,16 @@ grub_ata_pciinit (int bus, int device, i
>  	{
>  	  grub_ata_device_initialize (controller * 2 + i, 0, rega, regb);
>  	  grub_ata_device_initialize (controller * 2 + i, 1, rega, regb);
> +
> +	  /* Most errors rised by grub_ata_device_initialize() are harmless.
> +	     They just indicate this particular drive is not responding, most
> +	     likely because it doesn't exist.  We might want to ignore specific
> +	     error types here, instead of printing them.  */
> +	  if (grub_errno)
> +	    {
> +	      grub_print_error ();
> +	      grub_errno = GRUB_ERR_NONE;
> +	    }
>  	}
>      }
>  

> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


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