committed
On Fri, Jun 19, 2009 at 8:44 PM, Vladimir 'phcoder'
Serbinenko<phco...@gmail.com> wrote:
>
>
> On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin <pro...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
