Re: [PATCH] Add SELinux module to 2.5.74-bk1

[Christoph Hellwig <hch@infradead.org>]

On Tue, Jul 08, 2003 at 07:49:41PM +1000, James Morris wrote:
> +struct hashtab {
> +	struct hashtab_node **htable;	/* hash table */
> +	u32 size;			/* number of slots in hash table */
> +	u32 nel;			/* number of elements in hash table */
> +	u32 (*hash_value)(struct hashtab *h, void *key);
> +					/* hash function */
> +	int (*keycmp)(struct hashtab *h, void *key1, void *key2);
> +					/* key comparison function */

We use this callbacks in a bunch opf places, maybe add hash_value_t
and keycmp_t typedefs for them to avoid typing the prototypes all
the time?

> +/*
> +   Creates a new hash table with the specified characteristics.
> +
> +   Returns NULL if insufficent space is available or
> +   the new hash table otherwise.
> + */

Standrad kernel komments are either

/* foo */

or 

/*
 * Foo bar whiz blanbggvsgvb.
 */

> +struct hashtab *
> +hashtab_create(u32 (*hash_value)(struct hashtab *h, void *key),
> +               int (*keycmp)(struct hashtab *h, void *key1, void *key2),
> +               u32 size);

Documentation/CodingStyle says the type should be on the same line
as the function name.  I don't see that religious but it seems Linus
does according to a recent thread :)

> +/*
> +   Inserts the specified (key, datum) pair into the specified hash table.
> +
> +   Returns -ENOMEM on memory allocation error,
> +   -EEXIST if there is already an entry with the same key,
> +   -EINVAL for general errors or
> +   0 otherwise.
> + */

Maybe add kerneldoc comments in the source file instead of the
header comments?

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/hashtab.h>
> +
> +struct hashtab *
> +hashtab_create(u32 (*hash_value)(struct hashtab *h, void *key),
> +               int (*keycmp)(struct hashtab *h, void *key1, void *key2),
> +               u32 size)
> +{
> +	u32 i;
> +	struct hashtab *p;
> +
> +	p = kmalloc(sizeof(*p), GFP_KERNEL);

Pass the GFP_ mask down to hashtab_create() maybe?

> +	p->htable = kmalloc(sizeof(p) * size, GFP_KERNEL);
> +	if (p->htable == NULL) {
> +		kfree(p);
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < size; i++)
> +		p->htable[i] = NULL;

memset instead?

> +		memset(newnode, 0, sizeof(*newnode));
> +		newnode->key = key;
> +		newnode->datum = datum;
> +		if (prev) {
> +			newnode->next = prev->next;
> +			prev->next = newnode;

Use hlists?

> +config HASHTAB
> +	tristate "Generic hash table support"

Should this really be a user option or rather implicitly selected
by it's users?

>  CPPFLAGS	:= -D__KERNEL__ -Iinclude
>  CFLAGS 		:= $(CPPFLAGS) -Wall -Wstrict-prototypes -Wno-trigraphs -O2 \
> -	  	   -fno-strict-aliasing -fno-common
> +	  	   -fno-strict-aliasing -fno-common -g

accident?