Messages in this thread | | | Date | Fri, 11 Mar 2005 23:25:23 -0800 | From | Greg KH <> | Subject | Re: [PATCH 2/6] PCI Express Advanced Error Reporting Driver |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |