Re: [PATCH] CKRM 1/10: Base CKRM Events

[Gerrit Huizenga]

On Mon, 29 Nov 2004 13:33:10 PST, Greg KH wrote:
> On Mon, Nov 29, 2004 at 10:46:00AM -0800, Gerrit Huizenga wrote:
> > +++ linux-2.6.10-rc2/include/linux/ckrm_events.h	2004-11-19 20:40:52.517303823 -0800

[... snip ...]

> > +#ifdef CONFIG_CKRM
> 
> This ifdef is not needed.

Done - globally.

> > +enum ckrm_event {
> 
> Please use a __bitwise marker for your enum so that sparse will check
> for improper usage of it.
 

Added this to the todo list - we've played with sparse a bit but
I haven't figured out how to do it for enum's correctly yet.  But
will do so soon.

> > +#ifdef __KERNEL__
> 
> Not needed (see the recent thread on lkml for why).

Fixed globally (I think all of them were unnecessary in all of our
current patches - none of the header files are used by user level
apps today).

> > +typedef void (*ckrm_event_cb) (void *arg);
> > +
> > +struct ckrm_hook_cb {
> > +	ckrm_event_cb fct;
> > +	struct ckrm_hook_cb *next;
> 
> Why not use the kernel list structures here instead?
 
Good catch.  Added to TODO list.  This should be trivial but I didn't
get to it this round.

> > +#else /* !CONFIG_CKRM */
> 
> Why have 2 different sections in the same header file for this check?
> Please put them all together.

Yeah - this was stupid, fixed.

> > +#ifdef CONFIG_CKRM
> > +void ckrm_cb_newtask(struct task_struct *);
> > +void ckrm_cb_exit(struct task_struct *);
> > +#else
> > +#define ckrm_cb_newtask(x)	do { } while (0)
> > +#define ckrm_cb_exit(x)		do { } while (0)
> 
> Make these static inlines to get the compiler to check the arguments
> properly.
 
Yep - done.

> > +extern int get_exe_path_name(struct task_struct *, char *, int);
> 
> Should this really be here?
 
Nope.  Gone.

> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.10-rc2/kernel/ckrm/ckrm_events.c	2004-11-19 20:40:52.526302397 -0800
> > @@ -0,0 +1,97 @@
> > +/* ckrm_events.c - Class-based Kernel Resource Management (CKRM)
> > + *               - event handling routines
> > + *
> > + * Copyright (C) Hubertus Franke, IBM Corp. 2003, 2004
> > + *           (C) Chandra Seetharaman,  IBM Corp. 2003
> > + * 
> > + * 
> > + * Provides API for event registration and handling for different
> > + * classtypes.
> > + *
> > + * Latest version, more details at http://ckrm.sf.net
> > + * 
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + */
> > +
> > +/* Changes
> > + *
> > + * 29 Sep 2004
> > + *        Separated from ckrm.c
> > + *  
> > + */
> 
> No changelogs in files please.
 
Fixed globally.

> > +#define ECC_PRINTK(fmt, args...) \
> > +// printk("%s: " fmt, __FUNCTION__ , ## args)
> 
> Looks like it's unused to me :)
> 
> Use pr_debug() if you want debugging stuff, don't make up your own...

Fixed globally.

gerrit
-
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/