Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, May 29, 2014 at 09:40:39AM +0300, Dan Carpenter wrote:
> On Thu, May 29, 2014 at 09:17:09AM +0900, DaeSeok Youn wrote:
> > Hi, Dan.
> > 
> > 2014-05-28 19:11 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > > On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
> > >> > In your patch it has:
> > >> > +       dgap_tty_uninit(brd, false);
> > >> >
> > >> > But it should only be "false" if dgap_tty_init() failed.  If
> > >> > dgap_tty_register_ports() fails then it should be "true".  Another
> > >> Yes, you're right. There were no error handle for tty_port_register_device() and
> > >> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
> > >> It need to add error handlers for them, right?
> > >
> > > Eventually, yes.  But I don't see a simple way to fix
> > > dgap_firmware_load() until after the code is cleaned up.
> > >
> > >>
> > >> > problem is that as you say, the earlier function are allocating
> > >> > resources like dgap_tty_register() but only the last two function calls
> > >> > have a "goto err_cleanup;" so the error handling is incomplete.
> > >> So remove "goto" in dgap_firmware_load() and add error handler in
> > >> dgap_tty_init()
> > >
> > > In the current code there isn't a goto in dgap_firmware_load().  Remove
> > > the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
> > Yes. I will try to fix it.
> > >
> > > That will clean up the code, and fix some NULL dereference bugs inside
> > > dgap_tty_uninit().
> > >
> > >> and dgap_tty_register_ports(), right?
> > >
> > > Inside dgap_tty_register_ports(), then we should add a
> > > kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> > > That is not a complete fix, but it is a part fix and it is clean.
> > Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed
> > into staging-next branch.
> > see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in
> > dgap_tty_register_ports()
> > Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch.
> > 
> > Can I send a patch after revert "0ade4a34fd43" commit?
> 
> Oh, crud.  I missed that.  Yeah.  Let's revert it.

Now reverted.