From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1MHhKo-0001EH-64 for mharc-grub-devel@gnu.org; Fri, 19 Jun 2009 12:53:02 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MHhKl-0001AU-9s for grub-devel@gnu.org; Fri, 19 Jun 2009 12:52:59 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MHhKf-00014F-SM for grub-devel@gnu.org; Fri, 19 Jun 2009 12:52:57 -0400 Received: from [199.232.76.173] (port=48671 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MHhKf-000149-KK for grub-devel@gnu.org; Fri, 19 Jun 2009 12:52:53 -0400 Received: from c60.cesmail.net ([216.154.195.49]:20553) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.60) (envelope-from ) id 1MHhKf-0003Hh-1d for grub-devel@gnu.org; Fri, 19 Jun 2009 12:52:53 -0400 Received: from unknown (HELO smtprelay1.cesmail.net) ([192.168.1.111]) by c60.cesmail.net with ESMTP; 19 Jun 2009 12:52:51 -0400 Received: from [192.168.0.22] (static-72-92-88-10.phlapa.fios.verizon.net [72.92.88.10]) by smtprelay1.cesmail.net (Postfix) with ESMTPSA id 3014334C69 for ; Fri, 19 Jun 2009 12:52:50 -0400 (EDT) From: Pavel Roskin To: The development of GRUB 2 In-Reply-To: References: <1245424852.3431.30.camel@mj> Content-Type: text/plain Date: Fri, 19 Jun 2009 12:52:49 -0400 Message-Id: <1245430369.28417.46.camel@mj> Mime-Version: 1.0 X-Mailer: Evolution 2.26.2 (2.26.2-1.fc11) Content-Transfer-Encoding: 7bit X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. Subject: Re: [PATCH] fix SigSegV and hang with grub-emu-usb X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jun 2009 16:53:00 -0000 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