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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

regards,
dan carpenter