[lm-sensors] [patch] hwmon: fix unchecked returncodes in w83791d

[lm-sensors] [patch] hwmon: fix unchecked returncodes in w83791d

[Jim Cromie]

Fix unchecked return-status by replacing all unchecked calls 
to device_create_file with a single group declaration,
and one call to sysfs_create_group, and check that one return status.

Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>
---
$ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
 w83791d.c |   83 +++++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 55 insertions(+), 28 deletions(-)

---

Its impressive (or embarrassing) how much slop you detect.
I trust this comment is in right place for auto-strip to 
keep it out of Changelog.  Is any of this automated, and if so,
are we/I getting it right (or close) ?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-unchecked-return-status-fixes-w83791d.patch
Type: text/x-patch
Size: 3292 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060925/26f34a04/attachment.bin 

[lm-sensors] [patch] hwmon: fix unchecked returncodes in w83791d

[Jean Delvare]

> Fix unchecked return-status by replacing all unchecked calls 
> to device_create_file with a single group declaration,
> and one call to sysfs_create_group, and check that one return status.
> 
> Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>
> ---
> $ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
>  w83791d.c |   83 +++++++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 55 insertions(+), 28 deletions(-)
> 
> ---
> 
> Its impressive (or embarrassing) how much slop you detect.
> I trust this comment is in right place for auto-strip to 
> keep it out of Changelog.  Is any of this automated, and if so,
> are we/I getting it right (or close) ?

Yes, anything between "---" and the beginning of the actual patch is
dropped upstream. But I'm still editing things manually anyway so it's
no big deal where you add your comments.

> @@ -1029,6 +1054,8 @@ static int w83791d_detach_client(struct 
>  	if (data)
>  		hwmon_device_unregister(data->class_dev);
>  
> +	sysfs_remove_group(&client->dev.kobj, &w83791d_group);
> +
>  	if ((err = i2c_detach_client(client)))
>  		return err;

The "if (data)" is used to differenciate between the real client and
the subclients, exactly as is done in the w83781d driver. As subclients
have no files, the call to sysfs_remove_group() should be made
conditional as well. If not, it'll still work, but with a significant
performance drop.

Thanks,
-- 
Jean Delvare

[lm-sensors] [patch] hwmon: fix unchecked returncodes in w83791d

[Jim Cromie]

Jean Delvare wrote:
>> Fix unchecked return-status by replacing all unchecked calls 
>> to device_create_file with a single group declaration,
>> and one call to sysfs_create_group, and check that one return status.
>>
>> Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>
>> ---
>> $ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
>>  w83791d.c |   83 +++++++++++++++++++++++++++++++++++++++++---------------------
>>  1 files changed, 55 insertions(+), 28 deletions(-)
>>
>> ---
>>
>> Its impressive (or embarrassing) how much slop you detect.
>> I trust this comment is in right place for auto-strip to 
>> keep it out of Changelog.  Is any of this automated, and if so,
>> are we/I getting it right (or close) ?
>>     
>
> Yes, anything between "---" and the beginning of the actual patch is
> dropped upstream. But I'm still editing things manually anyway so it's
> no big deal where you add your comments.
>
>   
>> @@ -1029,6 +1054,8 @@ static int w83791d_detach_client(struct 
>>  	if (data)
>>  		hwmon_device_unregister(data->class_dev);
>>  
>> +	sysfs_remove_group(&client->dev.kobj, &w83791d_group);
>> +
>>  	if ((err = i2c_detach_client(client)))
>>  		return err;
>>     
>
> The "if (data)" is used to differenciate between the real client and
> the subclients, exactly as is done in the w83781d driver. As subclients
> have no files, the call to sysfs_remove_group() should be made
> conditional as well. If not, it'll still work, but with a significant
> performance drop.
>
>   
on this item, I actually checked :-)

 w83791d_detect will fail unless data is sucessfully allocated,
so it must be there if detach_client is called.

but I suppose its safer (less action-at-a-distance) your way, esp wrt 
any future changes.
redo attached.

> Thanks,
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-unchecked-return-status-fixes-w83791d.patch
Type: text/x-patch
Size: 3343 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060925/773e9a77/attachment.bin 

[lm-sensors] [patch] hwmon: fix unchecked returncodes in w83791d

[Jean Delvare]

Hi Jim,

> > > @@ -1029,6 +1054,8 @@ static int w83791d_detach_client(struct 
> > >  	if (data)
> > >  		hwmon_device_unregister(data->class_dev);
> > >  
> > > +	sysfs_remove_group(&client->dev.kobj, &w83791d_group);
> > > +
> > >  	if ((err = i2c_detach_client(client)))
> > >  		return err;
> >
> > The "if (data)" is used to differenciate between the real client and
> > the subclients, exactly as is done in the w83781d driver. As subclients
> > have no files, the call to sysfs_remove_group() should be made
> > conditional as well. If not, it'll still work, but with a significant
> > performance drop.
>
> on this item, I actually checked :-)
> 
>  w83791d_detect will fail unless data is sucessfully allocated,
> so it must be there if detach_client is called.
> 
> but I suppose its safer (less action-at-a-distance) your way, esp wrt 
> any future changes.
> redo attached.

I'm sorry to insist, but you overlooked another function which also
attaches clients: w83791d_create_subclient(). And this one does _not_
associate data to the client. All three clients (one main and two
subclients) have the same driver, so w83791d_detach_client() is called
for all of them. The "if (data)" test is there exactly for this reason,
so this test is really needed.

The new patch looks OK to me, thanks.

-- 
Jean Delvare