Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 8 Jul 2003 12:33:04 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH] Add SELinux module to 2.5.74-bk1 |
| |
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?
- 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/
| ![\](/images/icornerr.gif) |