Re: [PATCH 2/6] PCI Express Advanced Error Reporting Driver

[Greg KH <greg@kroah.com>]

On Fri, Mar 11, 2005 at 04:13:33PM -0800, long wrote:
> +static ssize_t aer_sysfs_consume_show(struct device_driver *dev, char *buf)
> +{
> +	return aer_fsprint_record(buf);
> +}
> +                  	
> +static ssize_t aer_sysfs_status_show(struct device_driver *dev, char *buf)
> +{
> +	return aer_fsprint_devices(buf);
> +}
> +                  	

Why call wrapper functions that only do one thing?  Why have this extra
layer of indirection that is not needed from what I can tell?

> +static ssize_t aer_sysfs_verbose_show(struct device_driver *dev, char *buf)
> +{
> +	return sprintf(buf, "Verbose display set to %d\n", 
> +		aer_get_verbose());				
> +}

Just echo the value, don't print out pretty strings :)

> +static ssize_t aer_sysfs_verbose_store(struct device_driver *drv, 	
> +					const char *buf, size_t count)  
> +{                            
> +	aer_set_verbose(buf[0] - 0x30);			
> +	return count;							
> +}

Oh, that's a problem waiting to happen... Please validate the user
provided value before acting on it.

> +static ssize_t aer_sysfs_auto_show(struct device_driver *dev, char *buf)
> +{
> +	return sprintf(buf, "Automatic reporting is %s\n", 		
> +		(aer_get_auto_mode()) ? "on" : "off");  			
> +}

Again, just print on/off.

> +static ssize_t aer_sysfs_auto_store(struct device_driver *drv, 	
> +					const char *buf, size_t count)          
> +{                            
> +	aer_set_auto_mode(buf[0] - 0x30);			
> +	return count;							
> +}

Also validate this.

thanks,

greg k-h