On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
>
>
> On Wed, 20 Dec 2017, Dan Carpenter wrote:
>
> > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > > dev_err(&client->dev, "gpio request/direction_output fail");
> > > goto fail2;
> > > }
> > > - if (ACPI_HANDLE(&client->dev))
> > > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > - return 0;
> > > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > fail2:
> > > media_entity_cleanup(&flash->sd.entity);
> > > v4l2_ctrl_handler_free(&flash->ctrl_handler);
> >
> > Actually every place where we directly return a function call is wrong
> > and needs error handling added. I've been meaning to write a Smatch
> > check for this because it's a common anti-pattern we don't check the
> > last function call for errors.
> >
> > Someone could probably do the same in Coccinelle if they want.
>
> I'm not sure what you are suggesting. Is every case of return f(...);
> for any f wrong? Or is it a particular function that is of concern? Or
> would it be that every function call that has error handling somewhere
> should have error handling everywhere? Or is it related to what seems to
> be the problem in the above code that err is initialized but nothing
> happens to it?
>
I was just thinking that it's a common pattern to treat the last
function call differently and one mistake I often see looks like this:
ret = frob();
if (ret) {
cleanup();
return ret;
}
return another_function();
No error handling for the last function call.
regards,
dan carpenter
