(ata.mod) avoid passing grub_errno to upper layer
PATCH 1/1 (ata.mod) avoid passing grub_errno to upper layer
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.
Signature · 5 lines
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-22 Robert Millan <[email protected]>
* disk/ata.c (grub_ata_device_initialize): Avoid passing grub_errno conditions to the upper layer unless they're critical.
ata.c +14 -14
@@ -421,19 +421,13 @@ grub_ata_device_initialize (int port, ingrub_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, inelse {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
Quoted 8 lines · 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
Quoted 6 lines · Yoshinori K. Okuji
Quoted 5 lines · 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).
Quoted 7 lines
Quoted 3 lines
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.
Signature · 5 lines
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."
Quoted 10 lines · Robert Millan
Quoted 9 lines · Yoshinori K. Okuji
Quoted 8 lines · 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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 20 lines · Vesa Jääskeläinen
Quoted 10 lines · Robert Millan
Quoted 9 lines · Yoshinori K. Okuji
Quoted 8 lines · 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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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() ?
Signature · 5 lines
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."
Okay, I think I got what you mean. So how about this one?
Signature · 5 lines
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 <[email protected]>
* disk/ata.c (grub_ata_pciinit): Handle errors rised by grub_ata_device_initialize() calls.
ata.c +10 -0
@@ -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 ; + } } }
Committed (after repeating the same block for both calls, as Vesa suggested on IRC).
Quoted 41 lines · Robert Millan
Okay, I think I got what you mean. So how about this one?
Signature · 13 lines
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 <[email protected]> * disk/ata.c (grub_ata_pciinit): Handle errors rised by grub_ata_device_initialize() calls. Index: disk/ata.c ===================================================================ata.c +10
-0@@ -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
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 5 lines
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."