On Fri, 2009-06-19 at 17:54 +0200, Vladimir 'phcoder' Serbinenko wrote:
> I thought it was clear. Here is an explanation hunk by hunk:
> diff --git a/disk/scsi.c b/disk/scsi.c
> index 046dcb8..312d58a 100644
> --- a/disk/scsi.c
> +++ b/disk/scsi.c
> @@ -246,8 +246,9 @@ grub_scsi_open (const char *name, grub_disk_t
> disk)
>
> for (p = grub_scsi_dev_list; p; p = p->next)
> {
> - if (! p->open (name, scsi))
> - {
> + if (p->open (name, scsi))
> + continue;
> +
> disk->id = (unsigned long) "scsi"; /* XXX */
> disk->data = scsi;
> scsi->dev = p;
>
> This is purely stilistic to avoid unnecessarily long if
It can be committed without review.
> @@ -266,7 +267,7 @@ grub_scsi_open (const char *name, grub_disk_t
> disk)
> {
> grub_free (scsi);
> grub_dprintf ("scsi", "inquiry failed\n");
> - return grub_errno;
> + return err;
> }
>
> grub_dprintf ("scsi", "inquiry: devtype=0x%02x removable=%d\n",
> Error wasn't propagated which caused double closing which resulted in
> sigsegv. With another hunk (adding grub-error) this hunk wouldn't be
> necessary but I consider construction
> err = ....;
> if (err)
> return err;
> more logical than
> err = ....;
> if (err)
> return grub_errno;
OK, I understand you tried USB mass storage devices.
I believe the paramount here is consistency. There are several places
in the code where grub_errno is returned. In one place, grub_error() is
returned. It's important to fix all places at once.
Also, please check other .open functions in other disk drivers. In
disk/fs_uuid.c, grub_error() is used. The same is in disk/host.c.
I see the standard is grub_error(). Let's do it for SCSI as well.
> --- a/disk/usbms.c
> +++ b/disk/usbms.c
> @@ -226,7 +226,7 @@ grub_usbms_transfer (struct grub_scsi *scsi,
> grub_size_t cmdsize, char *cmd,
>
> retry:
> if (retrycnt == 0)
> - return err;
> + return grub_error (GRUB_ERR_IO, "USB Mass Storage stalled");
>
> /* Setup the request. */
> grub_memset (&cbw, 0, sizeof (cbw));
>
> when retry numbers failed returned error was ERR_NONE even if nothing
> was read
Something is wrong with the logic in that function. retrycnt is only
changed in one place, and if it hits zero, we don't go to the retry
label. I think the condition can be removed. I don't see how your
change could have fixed anything in the behavior.
> @@ -246,6 +246,7 @@ grub_usbms_transfer (struct grub_scsi *scsi,
> grub_size_t cmdsize, char *cmd,
> if (err == GRUB_USB_ERR_STALL)
> {
> grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
> + retrycnt--;
> goto retry;
> }
> return grub_error (GRUB_ERR_IO, "USB Mass Storage request
> failed");;
> retrycnt wasn't decreased which caused grub2 to retry infinitely hence
> a hang.
There are many instances of "goto retry" where you don't decrement
retrycnt. Then let's decrement retrycnt in the beginning.
Generally, when making a change, please have a look if it needs to be
done elsewhere.
> --- a/util/usb.c
> +++ b/util/usb.c
> @@ -51,6 +51,7 @@ grub_libusb_devices (void)
> for (usbdev = bus->devices; usbdev; usbdev = usbdev->next)
> {
> struct usb_device_descriptor *desc = &usbdev->descriptor;
> + grub_err_t err;
>
> if (! desc->bcdUSB)
> continue;
> @@ -62,7 +63,9 @@ grub_libusb_devices (void)
> dev->data = usbdev;
>
> /* Fill in all descriptors. */
> - grub_usb_device_initialize (dev);
> + err = grub_usb_device_initialize (dev);
> + if (err)
> + continue;
>
> /* Register the device. */
> grub_usb_devs[last++] = dev;
> When device couldn'r be initialized (e.g. because of privilege
> problem) it was still added to list. Subsequent access created sigsegv
Fine with me.
> Regarding the compile warning fix, I would try to make
> grub_libusb_init() and grub_libusb_fini() appear in
> grub_emu_init.h
> rather than declare them elsewhere.
> I was inspired by previous example of disk subsystems:
> #ifdef GRUB_UTIL
> void grub_raid_init (void);
> void grub_raid_fini (void);
> void grub_lvm_init (void);
> void grub_lvm_fini (void);
> #endif
> file: include/grub/disk.h
I would check why it was needed.
--
Regards,
Pavel Roskin
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
