Re: [lm-sensors] [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Guenter Roeck <guenter.roeck@ericsson.com>]

On Sun, Sep 05, 2010 at 08:32:29AM -0400, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > Or other options could be:
> > 
> > 1. Just calling sysfs_add_file_to_group() without collecting returned error and
> > return 0 at the end (driver/pci/pcie/aspm.c does like this). The drawback is
> > there is no error logged if an unlikely errorr occurs. But user can see some
> > files are missing in sysfs.
> >
> > 2. Or collect errors in err1, err2, etc for each sysfs_add_file_to_group. At
> > the end, return -ENODEV(??) if any err1, err2, etc is not 0. This option makes
> > code unreasonable complex to handle unlikely errors.
> 
> Well, the usual way to handle errors is to abort the operation when it 
> occurs, and return the error code that sysfs_add_file_to_group() gave.
> 
> The error is not 'fatal' but missing sysfs files sure are confusing, and 
> might break user-land which depends on them. So we should either 
> initialize a driver fully - or not intialize it at all.
> 
> Now, a sub-case is the question whether to emit something more than the 
> return code from sysfs_add_file_to_group(). If it's exceedingly rare 
> (and subsequently poorly tested) then adding a WARN_ON_ONCE(ret) is OK - 
> but that error code should be returned.
> 
> Am i missing any detail?
> 
Unless I am missing something, the problem here is that if driver initialization
code returns an error, the driver won't be installed. If device entries are retained,
this will ultimately cause the kernel to crash.

>From my perspective, the usual handling is just fine - ie not install the driver
at all. Everything else just causes confusion.

Guenter