Re: [patch 5/12] lsm stacking v0.2: actual stacker module

[Greg KH]

On Thu, Jun 30, 2005 at 02:50:43PM -0500, serue@xxxxxxxxxx wrote:
> +/* variables to hold kobject/sysfs data */
> +static struct subsystem stacker_subsys;

Use decl_subsys please.

And yes, James is right, only value per sysfs file is allowed.

> +	result = subsystem_register(&stacker_subsys);

Why are you putting this at the root of sysfs.  It should go in
/sys/kernel/security/ right?  Please put it there.

> +	sysfs_create_file(&stacker_subsys.kset.kobj,
> +			&stacker_attr_lockdown.attr);
> +	sysfs_create_file(&stacker_subsys.kset.kobj,
> +			&stacker_attr_listmodules.attr);
> +	sysfs_create_file(&stacker_subsys.kset.kobj,
> +			&stacker_attr_stop_responding.attr);
> +	sysfs_create_file(&stacker_subsys.kset.kobj,
> +			&stacker_attr_unload.attr);

Hm, I think those functions return error values, you might want to check
them...

> +	sysfsfiles_registered = 1;

Why?  You know if this works or not, otherwise you would have failed
here.

> +	printk(KERN_NOTICE "LSM stacker registered as the primary "
> +			   "security module\n");

And everyone really wants to see this in their logs?

> +	if (unregister_security (&stacker_ops))
> +		printk(KERN_WARNING "Error unregistering LSM stacker.\n");
> +	else
> +		printk(KERN_WARNING "LSM stacker removed.\n");

Same here as above...

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/