On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> On Fri, Apr 10, 2015 at 05:49:55PM +0300, Dan Carpenter wrote:
> > On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote:
> <snip>
> > > +
> > > parport_default_sysctl_table.sysctl_header =
> > > register_sysctl_table(parport_default_sysctl_table.dev_dir);
> >
> > Should we return an error if this fails?
> not sure. but even if it fails it will not affect the normal functioning
> of the parallel port. but I will add that in the next WIP patch.
Probably, it's better to leave it as-is if you aren't sure. I was just
asking because I didn't know myself.
> >
> > > - return 0;
> > > + ret = parport_bus_init();
> > > + if (ret)
> > > + unregister_sysctl_table(parport_default_sysctl_table.
> > > + sysctl_header);
> >
> >
> > ret = parport_bus_init();
> > if (ret) {
> > unregister_sysctl_table(
> > parport_default_sysctl_table.sysctl_header);
> > return ret;
> > }
> >
> > return 0;
> do we need two returns here? parport_bus_init() will return 0 if it succeeds,
> so return ret will return either 0 or the error code whatever the case maybe.
Yes, they work the same, you're right. But the other style is better
and more robust.
I have been trying to explain this to people but "return 0;" is
beautiful code. Functions normally are a list of statements in a row
with exceptions for error handling. The last statement in the success
path should be "return 0;".
Don't mix error and success paths. I see a quite a few bugs like this
where the error handling doesn't have a return then later we add some
code at the end of the function and forget to add the return.
ret = parport_bus_init();
if (ret)
unregister_sysctl_table();
ret = something_else();
return ret;
> >
> >
> > > + return ret;
> > > }
> > >
> > > static void __exit parport_default_proc_unregister(void)
> > > @@ -570,6 +576,7 @@ static void __exit
> > > parport_default_proc_unregister(void)
> > > sysctl_header);
> > > parport_default_sysctl_table.sysctl_header = NULL;
> > > }
> > > + parport_bus_exit();
> >
> > Do we need this function? Can't we call bus_unregister() directly?
> no, we dont need. on similar reasoning we also donot need parport_bus_init().
> I will remove both. :)
> >
> <snip>
> >
> > > +struct bus_type parport_bus_type = {
> > > + .name = "parport",
> > > + .match = parport_match,
> >
> > There is no need for a match function. If it's NULL that's the same a
> > "return 1" fuction. This is called from driver_match_device().
> ok.
> >
> <snip>
> > > + ret = driver_register(&drv->driver);
> > > + if (ret < 0) {
> >
> > if (ret) {
> >
> > > + mutex_lock(®istration_lock);
> > > + list_del_init(&drv->list);
> > > + list_for_each_entry(port, &portlist, list)
> > > + drv->detach(port);
> >
> > Does this free port? Should this be list_for_each_entry_safe?
> I am not sure what you meant by "free port". attach will claim the port,
> and the port will be marked. detach will just remove that connection and
> the driver will release the port.
My concern is that we dereference port to get the next port. If it's
freed now it causes a use after free. It's easier to detect if you have
free poisoning turned on.
> >
> > > + mutex_unlock(®istration_lock);
> >
> > return ret;
> > > + }
> > > +
> > > + return ret;
> >
> > return 0;
> do we need two returns? as ret will be either 0 or error code.
> >
> > > }
> > >
> <snip>
> >
> > Please use "if (ret) " everywhere unless it returns positive on success.
> sure.
> >
> > I know that I have done a rubbish review. I'm going to have to review
> > this properly later.
> main thing i wanted to know is if my approach is correct. since nothing
> on that so I hope I am on the correct track. Thanks.
> I will send in the next version in a day or two.
Heh. No, I really know less than you do about the driver model at this
point. Sorry. It's going to take me a bit to get up to speed.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
