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