[PATCH] fix SigSegV and hang with grub-emu-usb

[PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

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

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
-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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;
@@ -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",
@@ -306,7 +307,6 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 
 	  return GRUB_ERR_NONE;
 	}
-    }
 
   grub_free (scsi);
 
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));
@@ -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");;
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 ();
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;

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Pavel Roskin]

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.

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.

-- 
Regards,
Pavel Roskin

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

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

On Fri, Jun 19, 2009 at 5:20 PM, Pavel Roskin <proski@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

[-- Attachment #2: Type: text/html, Size: 6513 bytes --]

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Pavel Roskin]

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

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 3418 bytes --]

On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin <proski@gnu.org> wrote:

> 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.
>
I don't understand what do you mean. grub_error () which don't come from
previous function

>
> 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.
>
Wel it didn't fixed the logic completely just one case when it was wrong.
Sorry that patch was low-quality. My goal was to enable everything by
default and the bugs in long-unmaintained libusb code weren't something I
wanted to spend time on.

>
> > @@ -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;
>
It was clearing grub_errno

>
>
> >         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.
>
It seems it's unnecessary. I removed them and it didn't generate any
warnings. Now I followed your recommendation and they build system with my
previous fixes picked it right

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



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #1.2: Type: text/html, Size: 5213 bytes --]

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

diff --git a/ChangeLog b/ChangeLog
index 4185723..69c8ae1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,22 +1,22 @@
 2009-06-17  Vladimir Serbinenko  <phcoder@gmail.com>
 
-	Enable all targets that can be built by default
+	Fix libusb
 
-	* configure.c: enable efiemu runtime, grub-emu, grub-emu-usb,
-	grub-mkfont and grub-fstest if they can be built
+	* Makefile.in (LIBUSB): new macro
+	* genmk.rb (Utility/print_tail): new method
+	(Utility/rule): use intermediary variable #{prefix}_OBJECTS
+	(top level): call util.print_tail at the end.
 
 2009-06-19  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* disk/scsi.c (grub_scsi_open): use continue instead of big if
 
-2009-06-17  Vladimir Serbinenko  <phcoder@gmail.com>
+2009-06-19  Vladimir Serbinenko  <phcoder@gmail.com>
 
-	Fix libusb
+	Enable all targets that can be built by default
 
-	* Makefile.in (LIBUSB): new macro
-	* genmk.rb (Utility/print_tail): new method
-	(Utility/rule): use intermediary variable #{prefix}_OBJECTS
-	(top level): call util.print_tail at the end.
+	* configure.c: enable efiemu runtime, grub-emu, grub-emu-usb,
+	grub-mkfont and grub-fstest if they can be built
 
 2009-06-18  Pavel Roskin  <proski@gnu.org>
 
diff --git a/disk/scsi.c b/disk/scsi.c
index 353e639..24ebdb6 100644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -248,6 +248,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
     {
       if (p->open (name, scsi))
 	continue;
+
       disk->id = (unsigned long) "scsi"; /* XXX */
       disk->data = scsi;
       scsi->dev = p;
@@ -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",
@@ -292,7 +293,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	{
 	  grub_free (scsi);
 	  grub_dprintf ("scsi", "READ CAPACITY failed\n");
-	  return grub_errno;
+	  return err;
 	}
 
       /* SCSI blocks can be something else than 512, although GRUB
diff --git a/disk/usbms.c b/disk/usbms.c
index 3c7ebaf..403ed19 100644
--- a/disk/usbms.c
+++ b/disk/usbms.c
@@ -222,11 +222,12 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_size_t cmdsize, char *cmd,
   struct grub_usbms_csw status;
   static grub_uint32_t tag = 0;
   grub_usb_err_t err = GRUB_USB_ERR_NONE;
-  int retrycnt = 3;
+  int retrycnt = 3 + 1;
 
  retry:
+  retrycnt--;
   if (retrycnt == 0)
-    return err;
+    return grub_error (GRUB_ERR_IO, "USB Mass Storage stalled");
 
   /* Setup the request.  */
   grub_memset (&cbw, 0, sizeof (cbw));
@@ -305,9 +306,7 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_size_t cmdsize, char *cmd,
       grub_usb_clear_halt (dev->dev, dev->in->endp_addr);
       grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
 
-      retrycnt--;
-      if (retrycnt)
-	goto retry;
+      goto retry;
     }
 
   if (status.status)
diff --git a/include/grub/disk.h b/include/grub/disk.h
index a47e113..8b56448 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -173,11 +173,4 @@ struct grub_disk_ata_pass_through_parms
 extern grub_err_t (* EXPORT_VAR(grub_disk_ata_pass_through)) (grub_disk_t,
 		   struct grub_disk_ata_pass_through_parms *);
 
-#ifdef GRUB_UTIL
-void grub_raid_init (void);
-void grub_raid_fini (void);
-void grub_lvm_init (void);
-void grub_lvm_fini (void);
-#endif
-
 #endif /* ! GRUB_DISK_HEADER */
diff --git a/util/grub-emu.c b/util/grub-emu.c
index c133dbe..badec18 100644
--- a/util/grub-emu.c
+++ b/util/grub-emu.c
@@ -192,10 +192,6 @@ main (int argc, char *argv[])
   /* XXX: This is a bit unportable.  */
   grub_util_biosdisk_init (dev_map);
 
-#if HAVE_USB_H
-  grub_libusb_init ();
-#endif
-
   grub_init_all ();
 
   /* Make sure that there is a root device.  */
diff --git a/util/usb.c b/util/usb.c
index e1d8c71..a687eea 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,12 @@ grub_libusb_devices (void)
 	  dev->data = usbdev;
 
 	  /* Fill in all descriptors.  */
-	  grub_usb_device_initialize (dev);
+	  err = grub_usb_device_initialize (dev);
+	  if (err)
+	    {
+	      grub_errno = GRUB_ERR_NONE;
+	      continue;
+	    }
 
 	  /* Register the device.  */
 	  grub_usb_devs[last++] = dev;
@@ -72,27 +78,6 @@ grub_libusb_devices (void)
   return GRUB_USB_ERR_NONE;
 }
 
-grub_err_t
-grub_libusb_init (void)
-{
-  usb_init();
-  usb_find_busses();
-  usb_find_devices();
-
-  if (grub_libusb_devices ())
-    return grub_errno;
-
-  grub_usb_controller_dev_register (&usb_controller);
-
-  return 0;
-}
-
-grub_err_t
-grub_libusb_fini (void)
-{
-  return 0;
-}
-
 \f
 int
 grub_usb_iterate (int (*hook) (grub_usb_device_t dev))
@@ -189,3 +174,22 @@ grub_usb_bulk_write (grub_usb_device_t dev,
   usb_close (devh);
   return GRUB_USB_ERR_STALL;
 }
+
+GRUB_MOD_INIT (libusb)
+{
+  usb_init();
+  usb_find_busses();
+  usb_find_devices();
+
+  if (grub_libusb_devices ())
+    return;
+
+  grub_usb_controller_dev_register (&usb_controller);
+
+  return;
+}
+
+GRUB_MOD_FINI (libusb)
+{
+  return;
+}

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

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

>
>> I would check why it was needed.
>>
> It seems it's unnecessary. I removed them and it didn't generate any
> warnings. Now I followed your recommendation and they build system with my
> previous fixes picked it right
>
These definitions were used before grub-probe used grub_init_all and
grub_fini_all. It should be safe to remove
And just ignore Changelog changes in make last patch

>
>
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: Type: text/html, Size: 2073 bytes --]

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Pavel Roskin]

On Fri, 2009-06-19 at 20:44 +0200, Vladimir 'phcoder' Serbinenko wrote:

>         I see the standard is grub_error().  Let's do it for SCSI as
>         well.
>         
> I don't understand what do you mean. grub_error () which don't come
> from previous function

You fixed some code in one place, but it's present in more than one
place in the same function.  Please either do it consistently or explain
why it's needed only in one place.

Also, I see that .open functions in files under /disk use grub_error()
to communicate errors to the caller.  Please explain why you want to do
it differently in scsi.c.

It's possible that the reasons are simple for somebody who reads the
code carefully, but if you are submitting the patch, it's important that
you demonstrate that you know what you are doing.

>         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.
>         
> Wel it didn't fixed the logic completely just one case when it was
> wrong. Sorry that patch was low-quality. My goal was to enable
> everything by default and the bugs in long-unmaintained libusb code
> weren't something I wanted to spend time on. 

The whole point in enabling more code is to catch such bugs and fix
them.  Fixing just the immediate obstacles makes the whole task
pointless, as it hides half or the problems.

Admittedly, I choked a warning in the escape sequence processing in
serial.c without fixing the escape sequence support, but the fix would
require a major rewrite, and I actually posted a message about the
problem.

> It seems it's unnecessary. I removed them and it didn't generate any
> warnings. Now I followed your recommendation and they build system
> with my previous fixes picked it right 

Good.

-- 
Regards,
Pavel Roskin

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

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

On Fri, Jun 19, 2009 at 11:47 PM, Pavel Roskin <proski@gnu.org> wrote:

> On Fri, 2009-06-19 at 20:44 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
> >         I see the standard is grub_error().  Let's do it for SCSI as
> >         well.
> >
> > I don't understand what do you mean. grub_error () which don't come
> > from previous function
>
> You fixed some code in one place, but it's present in more than one
> place in the same function.  Please either do it consistently or explain
> why it's needed only in one place.
>
Yes I know, I just haven't paid  enough attention. Sorry for this

>
> Also, I see that .open functions in files under /disk use grub_error()
> to communicate errors to the caller.  Please explain why you want to do
> it differently in scsi.c.
>
return err is used if err is an error code recieved from another function
which has already called grub_error (). It's the same in whole grub

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



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: Type: text/html, Size: 2217 bytes --]

Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Vladimir 'phcoder' Serbinenko]

committed

On Fri, Jun 19, 2009 at 8:44 PM, Vladimir 'phcoder'
Serbinenko<phcoder@gmail.com> wrote:
>
>
> On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin <proski@gnu.org> wrote:
>>
>> 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.
>
> I don't understand what do you mean. grub_error () which don't come from
> previous function
>>
>> 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.
>
> Wel it didn't fixed the logic completely just one case when it was wrong.
> Sorry that patch was low-quality. My goal was to enable everything by
> default and the bugs in long-unmaintained libusb code weren't something I
> wanted to spend time on.
>>
>> > @@ -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;
>
> It was clearing grub_errno
>>
>>
>> >         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.
>
> It seems it's unnecessary. I removed them and it didn't generate any
> warnings. Now I followed your recommendation and they build system with my
> previous fixes picked it right
>>
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git