But since you did write some sysfs code, might as well review it so you
know how to do it better next time around :)
On Wed, May 01, 2019 at 09:03:26PM -0700, ezemtsov@xxxxxxxxxx wrote:
> +static struct kobject *sysfs_root;
> +
> +static ssize_t version_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buff)
> +{
> + return snprintf(buff, PAGE_SIZE, "%d\n", INCFS_CORE_VERSION);
Hint about sysfs, it's "one value per file", so you NEVER care about the
size of the buffer because you "know" your single little number will
always fit.
So this should be:
return sprintf(buff, "%d\n", INCFS_CORE_VERSION);
Yes, code checkers hate it, send them my way, I'll be glad to point out
their folly :)
> +static struct kobj_attribute version_attr = __ATTR_RO(version);
> +
> +static struct attribute *attributes[] = {
> + &version_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group attr_group = {
> + .attrs = attributes,
> +};
ATTRIBUTE_GROUP()?
> +static int __init init_sysfs(void)
> +{
> + int res = 0;
No need to set to 0 here.
> +
> + sysfs_root = kobject_create_and_add(INCFS_NAME, fs_kobj);
> + if (!sysfs_root)
> + return -ENOMEM;
> +
> + res = sysfs_create_group(sysfs_root, &attr_group);
> + if (res) {
> + kobject_put(sysfs_root);
> + sysfs_root = NULL;
> + }
> + return res;
To be extra "fancy", there's no real need to create a kobject for your
filesystem if all you are doing is creating a subdir and some individual
attributes. Just add a "named" group to the parent kobject. Can be
done in a single line, no need for having to deal with fancy error
cases.
> +}
> +
> +static void cleanup_sysfs(void)
> +{
> + if (sysfs_root) {
> + sysfs_remove_group(sysfs_root, &attr_group);
> + kobject_put(sysfs_root);
> + sysfs_root = NULL;
Why set it to NULL?
> + }
> +}
thanks,
greg k-h
