[PATCH] lkdtm: cleanup headers and module_param/MODULE_PARM_DESC

[PATCH] lkdtm: cleanup headers and module_param/MODULE_PARM_DESC

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Fix module_param/sysfs file permission typo.

Clean up MODULE_PARM_DESC strings to avoid fancy (and incorrect)
formatting.

Fix header includes for lkdtm; add some needed ones, remove unused ones;
and fix this gcc warning:
drivers/misc/lkdtm.c:150: warning: 'struct buffer_head' declared inside parameter list
drivers/misc/lkdtm.c:150: warning: its scope is only this definition or declaration, which is probably not what you want

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/misc/lkdtm.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

--- linux-2619-rc2g8.orig/drivers/misc/lkdtm.c
+++ linux-2619-rc2g8/drivers/misc/lkdtm.c
@@ -44,12 +44,14 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/fs.h>
 #include <linux/module.h>
+#include <linux/buffer_head.h>
 #include <linux/kprobes.h>
-#include <linux/kallsyms.h>
+#include <linux/list.h>
 #include <linux/init.h>
-#include <linux/irq.h>
 #include <linux/interrupt.h>
+#include <linux/hrtimer.h>
 #include <scsi/scsi_cmnd.h>
 
 #ifdef CONFIG_IDE
@@ -116,16 +118,16 @@ static enum ctype cptype = NONE;
 static int count = DEFAULT_COUNT;
 
 module_param(recur_count, int, 0644);
-MODULE_PARM_DESC(recur_count, "Recurcion level for the stack overflow test,\
-				 default is 10");
+MODULE_PARM_DESC(recur_count, " Recursion level for the stack overflow test, "\
+				 "default is 10");
 module_param(cpoint_name, charp, 0644);
-MODULE_PARM_DESC(cpoint_name, "Crash Point, where kernel is to be crashed");
-module_param(cpoint_type, charp, 06444);
-MODULE_PARM_DESC(cpoint_type, "Crash Point Type, action to be taken on\
-				hitting the crash point");
-module_param(cpoint_count, int, 06444);
-MODULE_PARM_DESC(cpoint_count, "Crash Point Count, number of times the \
-				crash point is to be hit to trigger action");
+MODULE_PARM_DESC(cpoint_name, " Crash Point, where kernel is to be crashed");
+module_param(cpoint_type, charp, 0644);
+MODULE_PARM_DESC(cpoint_type, " Crash Point Type, action to be taken on "\
+				"hitting the crash point");
+module_param(cpoint_count, int, 0644);
+MODULE_PARM_DESC(cpoint_count, " Crash Point Count, number of times the "\
+				"crash point is to be hit to trigger action");
 
 unsigned int jp_do_irq(unsigned int irq)
 {


---
~Randy

Re: [PATCH] lkdtm: cleanup headers and module_param/MODULE_PARM_DESC

[Ankita Garg]

Hi,

>  #include <linux/kernel.h>
> +#include <linux/fs.h>
>  #include <linux/module.h>
> +#include <linux/buffer_head.h>
>  #include <linux/kprobes.h>
> -#include <linux/kallsyms.h>
> +#include <linux/list.h>
>  #include <linux/init.h>
> -#include <linux/irq.h>
>  #include <linux/interrupt.h>
> +#include <linux/hrtimer.h>
>  #include <scsi/scsi_cmnd.h>

Why does the module require fs.h, hrtimer.h, list.h and buffer_head.h? It works fine for me without these header files. I do not get any gcc warning. Moreover, I found that even init.h and interrupt.h are also not required.
  
> +MODULE_PARM_DESC(cpoint_name, " Crash Point, where kernel is to be crashed");
> +module_param(cpoint_type, charp, 0644);
> +MODULE_PARM_DESC(cpoint_type, " Crash Point Type, action to be taken on "\
> +				"hitting the crash point");
> +module_param(cpoint_count, int, 0644);
> +MODULE_PARM_DESC(cpoint_count, " Crash Point Count, number of times the "\
                                                                            ^^not required now!
> +				"crash point is to be hit to trigger action");
>  

Thanks for fixing the typo.
 
> ---
> ~Randy

-- 
Ankita Garg 
Linux Technology Center

Re: [PATCH] lkdtm: cleanup headers and module_param/MODULE_PARM_DESC

[Randy.Dunlap]

Ankita Garg wrote:
> Hi,
> 
>>  #include <linux/kernel.h>
>> +#include <linux/fs.h>
>>  #include <linux/module.h>
>> +#include <linux/buffer_head.h>
>>  #include <linux/kprobes.h>
>> -#include <linux/kallsyms.h>
>> +#include <linux/list.h>
>>  #include <linux/init.h>
>> -#include <linux/irq.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/hrtimer.h>
>>  #include <scsi/scsi_cmnd.h>
> 
> Why does the module require fs.h, hrtimer.h, list.h and buffer_head.h? It works fine for me without these header files. I do not get any gcc warning. Moreover, I found that even init.h and interrupt.h are also not required.

fs.h:  struct file *, struct block_device *
hrtimer.h:  struct hrtimer *
list.h:  struct list_head *
buffer_head.h:  struct buffer_head *

struct buffer_head is what triggered this.  gcc warns like this:

  CC [M]  drivers/misc/lkdtm.o
drivers/misc/lkdtm.c:150: warning: 'struct buffer_head' declared inside parameter list
drivers/misc/lkdtm.c:150: warning: its scope is only this definition or declaration, which is probably not what you want

or you could not #include those files and just do new source code lines like:

struct hrtimer;
struct buffer_head;
struct file;
struct block_dev;
struct list_head;

What we want to see is that source files explicly #include all header files
that they need or use, for structs, unions, extern data, function APIs, etc.
What is happening with lkdtm is that one or more header files is doing
this for you.  We want it to be more explicit than that.

-- 
~Randy