Hi Jean,
On Fri, Apr 15, 2011 at 03:37:58AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> Thanks for the review.
>
> On Thu, 14 Apr 2011 11:41:47 -0700, Guenter Roeck wrote:
> > On Wed, 2011-04-13 at 11:28 -0400, Jean Delvare wrote:
> > > @@ -590,6 +732,11 @@ static int __init i8k_init(void)
> > > if (!proc_i8k)
> > > return -ENOENT;
> > >
> > > + if (i8k_init_hwmon()) {
> > > + remove_proc_entry("i8k", NULL);
> > > + return -ENODEV;
> > > + }
> > > +
> > Should that be something like
> > err = i8k_init_hwmon();
> > if (err)
> > goto remove_proc;
> >
> > ...
> >
> > remove_proc:
> > remove_proc_entry("i8k", NULL);
> > return err;
>
> I would have done exactly this if it were my driver. However I noticed
> that the error code from i8k_probe() is not preserved, so I decided to
> follow the same logic for consistency.
>
Both are independent of each other. If a rule is not followed at one place
it doesn't mean that it should not be followed elsewhere. Besides, my main point
was to follow the one-function-exit rule; retaining the error code was a side
effect as
err = i8k_init_hwmon();
if (err) {
err = -ENODEV;
goto remove_proc;
}
didn't seem to make much sense.
Is there a generic rule that error codes shall be retained ? I looked for it,
but did not find it. If not I should add it to the hwmon driver guidelines.
> If you think this is a blocker, then I'll write a preliminary patch to
> preserve the error code returned by i8k_probe(), and then update my own
> patch in the way you suggested above.
>
I'll leave it up to you.
Acked-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
for both patches.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
