Re: [PATCH 8 of 18] ipath - sysfs and ipathfs support for core driver

[Greg KH <greg@kroah.com>]

On Wed, Mar 22, 2006 at 04:05:01PM -0800, Bryan O'Sullivan wrote:
> +int ipath_driver_create_group(struct device_driver *drv)
> +{
> +	int ret;
> +
> +	if (!drv->kobj.dentry) {
> +		ret = -ENODEV;
> +		goto bail;
> +	}
> +
> +	ret = sysfs_create_group(&drv->kobj, &driver_attr_group);
> +	if (ret)
> +		goto bail;
> +
> +	ret = sysfs_create_group(&drv->kobj, &driver_stat_attr_group);
> +	if (ret)
> +		sysfs_remove_group(&drv->kobj, &driver_attr_group);
> +
> +bail:
> +	return ret;
> +}
> +
> +void ipath_driver_remove_group(struct device_driver *drv)
> +{
> +	if (drv->kobj.dentry) {
> +		sysfs_remove_group(&drv->kobj, &driver_stat_attr_group);
> +		sysfs_remove_group(&drv->kobj, &driver_attr_group);
> +	}
> +}

Why are you testing kobj.dentry in these functions?  That test would not
have been valid in the mainline kernel until a day or so ago, so you
couldn't have ever hit that path (dentry being NULL that is.)

Or did you do that because of something odd you saw in sysfs?

Unless you did, I don't think you need these tests.

Oh, and I like your new filesystem, but where do you propose that it be
mounted?

> +int ipath_device_create_group(struct device *dev, struct ipath_devdata *dd)
> +{
> +	int ret;
> +	char unit[5];
> +
> +	ret = sysfs_create_group(&dev->kobj, &dev_attr_group);
> +	if (ret)
> +		goto bail;
> +
> +	ret = sysfs_create_group(&dev->kobj, &dev_counter_attr_group);
> +	if (ret)
> +		goto bail;
> +
> +	snprintf(unit, sizeof(unit), "%02d", dd->ipath_unit);
> +	ret = sysfs_create_link(&dev->driver->kobj, &dev->kobj, unit);
> +bail:
> +	return ret;
> +}

You leak a group if the second call to sysfs_create_group() fails for
some reason.

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/