On Fri, Jun 19, 2009 at 5:20 PM, Pavel Roskin <pro...@gnu.org> wrote:
> On Fri, 2009-06-19 at 16:58 +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Hello when testing grub-emu with USB support I stumbled across several
> problems
> > 1) compile time warning of undefined grub_usb_libinit
> > 2) When launched under normal user it crashed
> > 3) When launched as superuser it hanged on ls
> > Here is the fix. Formatting omitted for readability
>
> It's not clear which changes are responsible for fixing which problems.
> Please post separate patches if you want a meaningful review.
>
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
@@ -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;
@@ -306,7 +307,6 @@ grub_scsi_open (const char *name, grub_disk_t disk)
return GRUB_ERR_NONE;
}
- }
grub_free (scsi);
Counterpart of first hunk
diff --git a/disk/usbms.c b/disk/usbms.c
index 3c7ebaf..f67f918 100644
--- 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
@@ -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.
diff --git a/include/grub/usb.h b/include/grub/usb.h
index 8dd3b6e..d6d9a3e 100644
--- a/include/grub/usb.h
+++ b/include/grub/usb.h
@@ -204,4 +204,9 @@ grub_usb_get_config_interface (struct
grub_usb_desc_config *config)
return interf;
}
+#ifdef GRUB_UTIL
+grub_err_t grub_libusb_init (void);
+grub_err_t grub_libusb_fini (void);
+#endif
+
#endif /* GRUB_USB_H */
diff --git a/util/grub-emu.c b/util/grub-emu.c
index c133dbe..2621d18 100644
--- a/util/grub-emu.c
+++ b/util/grub-emu.c
@@ -39,6 +39,10 @@
#include <grub_emu_init.h>
+#if HAVE_USB_H
+#include <grub/usb.h>
+#endif
+
/* Used for going back to the main function. */
jmp_buf main_env;
@@ -223,6 +227,10 @@ main (int argc, char *argv[])
if (setjmp (main_env) == 0)
grub_main ();
+#if HAVE_USB_H
+ grub_libusb_fini ();
+#endif
+
grub_fini_all ();
grub_machine_fini ();
Previous hunks just fixed warnings
diff --git a/util/usb.c b/util/usb.c
index e1d8c71..4ca1c10 100644
--- 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
>
> 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
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
--
Regards
Vladimir 'phcoder' Serbinenko
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
