[PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Manish Katiyar]

Hi Ingo,

I used your code-quality script to do cleanup in kernel/kallsyms.c.
Below patch removes errors generated by checkpatch.pl.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/kallsyms.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 7b8b0f2..8dad1c3 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -93,13 +93,13 @@ static unsigned int
kallsyms_expand_symbol(unsigned int off, char *result)

        /* for every byte on the compressed symbol data, copy the table
           entry for that byte */
-       while(len) {
-               tptr = &kallsyms_token_table[ kallsyms_token_index[*data] ];
+       while (len) {
+               tptr = &kallsyms_token_table[kallsyms_token_index[*data]];
                data++;
                len--;

                while (*tptr) {
-			if(skipped_first) {
+			if (skipped_first) {
 				*result = *tptr;
 				result++;
 			} else
@@ -120,7 +120,7 @@ static char kallsyms_get_symbol_type(unsigned int off)
 {
 	/* get just the first code, look it up in the token table, and return the
 	 * first char from this token */
-	return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
+	return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];
 }


@@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)

 	/* use the closest marker we have. We have markers every 256 positions,
 	 * so that should be close enough */
-	name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
+	name = &kallsyms_names[kallsyms_markers[pos>>8]];

 	/* sequentially scan all the symbols up to the point we're searching for.
 	 * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
 	 * just need to add the len to the current pointer for every symbol we
 	 * wish to skip */
-	for(i = 0; i < (pos&0xFF); i++)
+	for (i = 0; i < (pos&0xFF); i++)
 		name = name + (*name) + 1;

 	return name - kallsyms_names;
@@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)

 	return len;
 }
+EXPORT_SYMBOL_GPL(sprint_symbol);

 /* Look up a kernel symbol and print it to the kernel messages. */
 void __print_symbol(const char *fmt, unsigned long address)
@@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
long address)

 	printk(fmt, buffer);
 }
+EXPORT_SYMBOL(__print_symbol);

 /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
-struct kallsym_iter
-{
+struct kallsym_iter {
 	loff_t pos;
 	unsigned long value;
 	unsigned int nameoff; /* If iterating in core kernel symbols */
@@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
loff_t pos)
 		iter->pos = pos;
 		return get_ksymbol_mod(iter);
 	}
-	
+
 	/* If we're not on the desired position, reset to new position. */
 	if (pos != iter->pos)
 		reset_iter(iter, pos);
@@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
 {
 	struct kallsym_iter *iter = m->private;

-	/* Some debugging symbols have no name.  Ignore them. */
+	/* Some debugging symbols have no name.  Ignore them. */
 	if (!iter->name[0])
 		return 0;

@@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
 		type = iter->exported ? toupper(iter->type) :
 					tolower(iter->type);
 		seq_printf(m, "%0*lx %c %s\t[%s]\n",
-			   (int)(2*sizeof(void*)),
+			   (int)(2*sizeof(void *)),
 			   iter->value, type, iter->name, iter->module_name);
 	} else
 		seq_printf(m, "%0*lx %c %s\n",
-			   (int)(2*sizeof(void*)),
+			   (int)(2*sizeof(void *)),
 			   iter->value, iter->type, iter->name);
 	return 0;
 }
@@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
 	proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
 	return 0;
 }
-__initcall(kallsyms_init);
+device_initcall(kallsyms_init);

-EXPORT_SYMBOL(__print_symbol);
-EXPORT_SYMBOL_GPL(sprint_symbol);
-- 
1.5.4.3


Thanks -
Manish

[PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Manish Katiyar]

Hi,

Below patch removes errors generated by checkpatch.pl in
kernel/utsname_sysctl.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/utsname_sysctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 3b34b35..d1dc4ca 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
write, struct file *filp,
 	int r;
 	memcpy(&uts_table, table, sizeof(uts_table));
 	uts_table.data = get_uts(table, write);
-	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
+	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
 	put_uts(table, write, uts_table.data);
 	return r;
 }
@@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
 	return 0;
 }

-__initcall(utsname_sysctl_init);
+device_initcall(utsname_sysctl_init);
-- 
1.5.4.3


Thanks -
Manish

[PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Manish Katiyar]

Hi Ingo,

I used your code-quality script to do cleanup in kernel/kallsyms.c.
Below patch removes errors generated by checkpatch.pl.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/kallsyms.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 7b8b0f2..8dad1c3 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -93,13 +93,13 @@ static unsigned int
kallsyms_expand_symbol(unsigned int off, char *result)

        /* for every byte on the compressed symbol data, copy the table
           entry for that byte */
-       while(len) {
-               tptr = &kallsyms_token_table[ kallsyms_token_index[*data] ];
+       while (len) {
+               tptr = &kallsyms_token_table[kallsyms_token_index[*data]];
                data++;
                len--;

                while (*tptr) {
-			if(skipped_first) {
+			if (skipped_first) {
 				*result = *tptr;
 				result++;
 			} else
@@ -120,7 +120,7 @@ static char kallsyms_get_symbol_type(unsigned int off)
 {
 	/* get just the first code, look it up in the token table, and return the
 	 * first char from this token */
-	return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
+	return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];
 }


@@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)

 	/* use the closest marker we have. We have markers every 256 positions,
 	 * so that should be close enough */
-	name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
+	name = &kallsyms_names[kallsyms_markers[pos>>8]];

 	/* sequentially scan all the symbols up to the point we're searching for.
 	 * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
 	 * just need to add the len to the current pointer for every symbol we
 	 * wish to skip */
-	for(i = 0; i < (pos&0xFF); i++)
+	for (i = 0; i < (pos&0xFF); i++)
 		name = name + (*name) + 1;

 	return name - kallsyms_names;
@@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)

 	return len;
 }
+EXPORT_SYMBOL_GPL(sprint_symbol);

 /* Look up a kernel symbol and print it to the kernel messages. */
 void __print_symbol(const char *fmt, unsigned long address)
@@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
long address)

 	printk(fmt, buffer);
 }
+EXPORT_SYMBOL(__print_symbol);

 /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
-struct kallsym_iter
-{
+struct kallsym_iter {
 	loff_t pos;
 	unsigned long value;
 	unsigned int nameoff; /* If iterating in core kernel symbols */
@@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
loff_t pos)
 		iter->pos = pos;
 		return get_ksymbol_mod(iter);
 	}
-	
+
 	/* If we're not on the desired position, reset to new position. */
 	if (pos != iter->pos)
 		reset_iter(iter, pos);
@@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
 {
 	struct kallsym_iter *iter = m->private;

-	/* Some debugging symbols have no name.  Ignore them. */
+	/* Some debugging symbols have no name.  Ignore them. */
 	if (!iter->name[0])
 		return 0;

@@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
 		type = iter->exported ? toupper(iter->type) :
 					tolower(iter->type);
 		seq_printf(m, "%0*lx %c %s\t[%s]\n",
-			   (int)(2*sizeof(void*)),
+			   (int)(2*sizeof(void *)),
 			   iter->value, type, iter->name, iter->module_name);
 	} else
 		seq_printf(m, "%0*lx %c %s\n",
-			   (int)(2*sizeof(void*)),
+			   (int)(2*sizeof(void *)),
 			   iter->value, iter->type, iter->name);
 	return 0;
 }
@@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
 	proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
 	return 0;
 }
-__initcall(kallsyms_init);
+device_initcall(kallsyms_init);

-EXPORT_SYMBOL(__print_symbol);
-EXPORT_SYMBOL_GPL(sprint_symbol);
-- 
1.5.4.3


Thanks -
Manish

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Sam Ravnborg]

On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> Hi Ingo,
> 
> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> Below patch removes errors generated by checkpatch.pl.
When doing so use checkpatch only as a hint generator and do
not concentrate only on the warnings/errors generated by checkpatch.

Your patch is an improvement but please fix the remaining issues.

	Sam

For example..
> -	return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
> +	return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];

Add spaces around [off+1] so it looks like this: [off + 1]


>  }
> 
> 
> @@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)
> 
>  	/* use the closest marker we have. We have markers every 256 positions,
>  	 * so that should be close enough */
> -	name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
> +	name = &kallsyms_names[kallsyms_markers[pos>>8]];

Same here: [pos >> 8]


> 
>  	/* sequentially scan all the symbols up to the point we're searching for.
>  	 * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
>  	 * just need to add the len to the current pointer for every symbol we
>  	 * wish to skip */
> -	for(i = 0; i < (pos&0xFF); i++)
> +	for (i = 0; i < (pos&0xFF); i++)
And here: (pos & 0xFF)

>  		name = name + (*name) + 1;
> 
>  	return name - kallsyms_names;
> @@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)
> 
>  	return len;
>  }
> +EXPORT_SYMBOL_GPL(sprint_symbol);
> 
>  /* Look up a kernel symbol and print it to the kernel messages. */
>  void __print_symbol(const char *fmt, unsigned long address)
> @@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
> long address)
> 
>  	printk(fmt, buffer);
>  }
> +EXPORT_SYMBOL(__print_symbol);
> 
>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
> -struct kallsym_iter
> -{
> +struct kallsym_iter {
>  	loff_t pos;
>  	unsigned long value;
>  	unsigned int nameoff; /* If iterating in core kernel symbols */
> @@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
> loff_t pos)
>  		iter->pos = pos;
>  		return get_ksymbol_mod(iter);
>  	}
> -	
> +
>  	/* If we're not on the desired position, reset to new position. */
>  	if (pos != iter->pos)
>  		reset_iter(iter, pos);
> @@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
>  {
>  	struct kallsym_iter *iter = m->private;
> 
> -	/* Some debugging symbols have no name.  Ignore them. */
> +	/* Some debugging symbols have no name.  Ignore them. */
>  	if (!iter->name[0])
>  		return 0;
> 
> @@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
>  		type = iter->exported ? toupper(iter->type) :
>  					tolower(iter->type);
>  		seq_printf(m, "%0*lx %c %s\t[%s]\n",
> -			   (int)(2*sizeof(void*)),
> +			   (int)(2*sizeof(void *)),
(2 * sizeof(void *))


>  			   iter->value, type, iter->name, iter->module_name);
>  	} else
>  		seq_printf(m, "%0*lx %c %s\n",
> -			   (int)(2*sizeof(void*)),
> +			   (int)(2*sizeof(void *)),
and here again.

>  			   iter->value, iter->type, iter->name);
>  	return 0;
>  }
> @@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
>  	proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
>  	return 0;
>  }
> -__initcall(kallsyms_init);
> +device_initcall(kallsyms_init);
> 
> -EXPORT_SYMBOL(__print_symbol);
> -EXPORT_SYMBOL_GPL(sprint_symbol);

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Sam Ravnborg]

On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> Hi Ingo,
> 
> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> Below patch removes errors generated by checkpatch.pl.
When doing so use checkpatch only as a hint generator and do
not concentrate only on the warnings/errors generated by checkpatch.

Your patch is an improvement but please fix the remaining issues.

	Sam

For example..
> -	return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
> +	return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];

Add spaces around [off+1] so it looks like this: [off + 1]


>  }
> 
> 
> @@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)
> 
>  	/* use the closest marker we have. We have markers every 256 positions,
>  	 * so that should be close enough */
> -	name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
> +	name = &kallsyms_names[kallsyms_markers[pos>>8]];

Same here: [pos >> 8]


> 
>  	/* sequentially scan all the symbols up to the point we're searching for.
>  	 * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
>  	 * just need to add the len to the current pointer for every symbol we
>  	 * wish to skip */
> -	for(i = 0; i < (pos&0xFF); i++)
> +	for (i = 0; i < (pos&0xFF); i++)
And here: (pos & 0xFF)

>  		name = name + (*name) + 1;
> 
>  	return name - kallsyms_names;
> @@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)
> 
>  	return len;
>  }
> +EXPORT_SYMBOL_GPL(sprint_symbol);
> 
>  /* Look up a kernel symbol and print it to the kernel messages. */
>  void __print_symbol(const char *fmt, unsigned long address)
> @@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
> long address)
> 
>  	printk(fmt, buffer);
>  }
> +EXPORT_SYMBOL(__print_symbol);
> 
>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
> -struct kallsym_iter
> -{
> +struct kallsym_iter {
>  	loff_t pos;
>  	unsigned long value;
>  	unsigned int nameoff; /* If iterating in core kernel symbols */
> @@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
> loff_t pos)
>  		iter->pos = pos;
>  		return get_ksymbol_mod(iter);
>  	}
> -	
> +
>  	/* If we're not on the desired position, reset to new position. */
>  	if (pos != iter->pos)
>  		reset_iter(iter, pos);
> @@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
>  {
>  	struct kallsym_iter *iter = m->private;
> 
> -	/* Some debugging symbols have no name.  Ignore them. */
> +	/* Some debugging symbols have no name.  Ignore them. */
>  	if (!iter->name[0])
>  		return 0;
> 
> @@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
>  		type = iter->exported ? toupper(iter->type) :
>  					tolower(iter->type);
>  		seq_printf(m, "%0*lx %c %s\t[%s]\n",
> -			   (int)(2*sizeof(void*)),
> +			   (int)(2*sizeof(void *)),
(2 * sizeof(void *))


>  			   iter->value, type, iter->name, iter->module_name);
>  	} else
>  		seq_printf(m, "%0*lx %c %s\n",
> -			   (int)(2*sizeof(void*)),
> +			   (int)(2*sizeof(void *)),
and here again.

>  			   iter->value, iter->type, iter->name);
>  	return 0;
>  }
> @@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
>  	proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
>  	return 0;
>  }
> -__initcall(kallsyms_init);
> +device_initcall(kallsyms_init);
> 
> -EXPORT_SYMBOL(__print_symbol);
> -EXPORT_SYMBOL_GPL(sprint_symbol);

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Manish Katiyar]

On Mon, Feb 16, 2009 at 12:17 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> Hi Ingo,
>>
>> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> Below patch removes errors generated by checkpatch.pl.
> When doing so use checkpatch only as a hint generator and do
> not concentrate only on the warnings/errors generated by checkpatch.
>
> Your patch is an improvement but please fix the remaining issues.
>
>        Sam

Thanks Sam, will address these and resend this patch.

Thanks -
Manish


>
> For example..
>> -     return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
>> +     return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];
>
> Add spaces around [off+1] so it looks like this: [off + 1]
>
>
>>  }
>>
>>
>> @@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)
>>
>>       /* use the closest marker we have. We have markers every 256 positions,
>>        * so that should be close enough */
>> -     name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
>> +     name = &kallsyms_names[kallsyms_markers[pos>>8]];
>
> Same here: [pos >> 8]
>
>
>>
>>       /* sequentially scan all the symbols up to the point we're searching for.
>>        * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
>>        * just need to add the len to the current pointer for every symbol we
>>        * wish to skip */
>> -     for(i = 0; i < (pos&0xFF); i++)
>> +     for (i = 0; i < (pos&0xFF); i++)
> And here: (pos & 0xFF)
>
>>               name = name + (*name) + 1;
>>
>>       return name - kallsyms_names;
>> @@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)
>>
>>       return len;
>>  }
>> +EXPORT_SYMBOL_GPL(sprint_symbol);
>>
>>  /* Look up a kernel symbol and print it to the kernel messages. */
>>  void __print_symbol(const char *fmt, unsigned long address)
>> @@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
>> long address)
>>
>>       printk(fmt, buffer);
>>  }
>> +EXPORT_SYMBOL(__print_symbol);
>>
>>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
>> -struct kallsym_iter
>> -{
>> +struct kallsym_iter {
>>       loff_t pos;
>>       unsigned long value;
>>       unsigned int nameoff; /* If iterating in core kernel symbols */
>> @@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
>> loff_t pos)
>>               iter->pos = pos;
>>               return get_ksymbol_mod(iter);
>>       }
>> -
>> +
>>       /* If we're not on the desired position, reset to new position. */
>>       if (pos != iter->pos)
>>               reset_iter(iter, pos);
>> @@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
>>  {
>>       struct kallsym_iter *iter = m->private;
>>
>> -     /* Some debugging symbols have no name.  Ignore them. */
>> +     /* Some debugging symbols have no name.  Ignore them. */
>>       if (!iter->name[0])
>>               return 0;
>>
>> @@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
>>               type = iter->exported ? toupper(iter->type) :
>>                                       tolower(iter->type);
>>               seq_printf(m, "%0*lx %c %s\t[%s]\n",
>> -                        (int)(2*sizeof(void*)),
>> +                        (int)(2*sizeof(void *)),
> (2 * sizeof(void *))
>
>
>>                          iter->value, type, iter->name, iter->module_name);
>>       } else
>>               seq_printf(m, "%0*lx %c %s\n",
>> -                        (int)(2*sizeof(void*)),
>> +                        (int)(2*sizeof(void *)),
> and here again.
>
>>                          iter->value, iter->type, iter->name);
>>       return 0;
>>  }
>> @@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
>>       proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
>>       return 0;
>>  }
>> -__initcall(kallsyms_init);
>> +device_initcall(kallsyms_init);
>>
>> -EXPORT_SYMBOL(__print_symbol);
>> -EXPORT_SYMBOL_GPL(sprint_symbol);
>
>

[PATCH] Remove errors caught by checkpatch.pl in kernel/acct.c

[Manish Katiyar]

Hi,

Below patch removes errors generated by checkpatch.pl in
kernel/acct.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/acct.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index 7afa315..00fbf8e 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -144,7 +144,7 @@ static int check_free_space(struct bsd_acct_struct
*acct, struct file *file)
        spin_lock(&acct_lock);
        if (file != acct->file) {
                if (act)
-                       res = act>0;
+                       res = act > 0;
                goto out;
        }

@@ -287,7 +287,7 @@ SYSCALL_DEFINE1(acct, const char __user *, name)
        if (name) {
                char *tmp = getname(name);
                if (IS_ERR(tmp))
-                       return (PTR_ERR(tmp));
+                       return PTR_ERR(tmp);
                error = acct_on(tmp);
                putname(tmp);
        } else {
@@ -403,7 +403,7 @@ static comp_t encode_comp_t(unsigned long value)
        return exp;
 }

-#if ACCT_VERSION==1 || ACCT_VERSION==2
+#if ACCT_VERSION == 1 || ACCT_VERSION == 2
 /*
  * encode an u64 into a comp2_t (24 bits)
  *
@@ -416,7 +416,7 @@ static comp_t encode_comp_t(unsigned long value)
 #define MANTSIZE2       20                      /* 20 bit mantissa. */
 #define EXPSIZE2        5                       /* 5 bit base 2 exponent. */
 #define MAXFRACT2       ((1ul << MANTSIZE2) - 1) /* Maximum
fractional value. */
-#define MAXEXP2         ((1 <<EXPSIZE2) - 1)    /* Maximum exponent. */
+#define MAXEXP2         ((1 << EXPSIZE2) - 1)    /* Maximum exponent. */

 static comp2_t encode_comp2_t(u64 value)
 {
@@ -447,7 +447,7 @@ static comp2_t encode_comp2_t(u64 value)
 }
 #endif

-#if ACCT_VERSION==3
+#if ACCT_VERSION == 3
 /*
  * encode an u64 into a 32 bit IEEE float
  */
@@ -456,8 +456,9 @@ static u32 encode_float(u64 value)
 	unsigned exp = 190;
 	unsigned u;

-	if (value==0) return 0;
-	while ((s64)value > 0){
+	if (value == 0)
+		return 0;
+	while ((s64)value > 0) {
 		value <<= 1;
 		exp--;
 	}
@@ -513,13 +514,13 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 		       + current->group_leader->start_time.tv_nsec;
 	/* convert nsec -> AHZ */
 	elapsed = nsec_to_AHZ(run_time);
-#if ACCT_VERSION==3
+#if ACCT_VERSION == 3
 	ac.ac_etime = encode_float(elapsed);
 #else
 	ac.ac_etime = encode_comp_t(elapsed < (unsigned long) -1l ?
-	                       (unsigned long) elapsed : (unsigned long) -1l);
+				(unsigned long) elapsed:(unsigned long) -1l);
 #endif
-#if ACCT_VERSION==1 || ACCT_VERSION==2
+#if ACCT_VERSION == 1 || ACCT_VERSION == 2
 	{
 		/* new enlarged etime field */
 		comp2_t etime = encode_comp2_t(elapsed);
@@ -531,15 +532,15 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 	ac.ac_btime = get_seconds() - elapsed;
 	/* we really need to bite the bullet and change layout */
 	current_uid_gid(&ac.ac_uid, &ac.ac_gid);
-#if ACCT_VERSION==2
+#if ACCT_VERSION == 2
 	ac.ac_ahz = AHZ;
 #endif
-#if ACCT_VERSION==1 || ACCT_VERSION==2
+#if ACCT_VERSION == 1 || ACCT_VERSION == 2
 	/* backward-compatible 16 bit fields */
 	ac.ac_uid16 = ac.ac_uid;
 	ac.ac_gid16 = ac.ac_gid;
 #endif
-#if ACCT_VERSION==3
+#if ACCT_VERSION == 3
 	ac.ac_pid = task_tgid_nr_ns(current, ns);
 	rcu_read_lock();
 	ac.ac_ppid = task_tgid_nr_ns(rcu_dereference(current->real_parent), ns);
-- 
1.5.4.3

Thanks -
Manish

[PATCH] Remove errors caught by checkpatch.pl in kernel/acct.c

[Manish Katiyar]

Hi,

Below patch removes errors generated by checkpatch.pl in
kernel/acct.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/acct.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index 7afa315..00fbf8e 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -144,7 +144,7 @@ static int check_free_space(struct bsd_acct_struct
*acct, struct file *file)
        spin_lock(&acct_lock);
        if (file != acct->file) {
                if (act)
-                       res = act>0;
+                       res = act > 0;
                goto out;
        }

@@ -287,7 +287,7 @@ SYSCALL_DEFINE1(acct, const char __user *, name)
        if (name) {
                char *tmp = getname(name);
                if (IS_ERR(tmp))
-                       return (PTR_ERR(tmp));
+                       return PTR_ERR(tmp);
                error = acct_on(tmp);
                putname(tmp);
        } else {
@@ -403,7 +403,7 @@ static comp_t encode_comp_t(unsigned long value)
        return exp;
 }

-#if ACCT_VERSION=1 || ACCT_VERSION=2
+#if ACCT_VERSION = 1 || ACCT_VERSION = 2
 /*
  * encode an u64 into a comp2_t (24 bits)
  *
@@ -416,7 +416,7 @@ static comp_t encode_comp_t(unsigned long value)
 #define MANTSIZE2       20                      /* 20 bit mantissa. */
 #define EXPSIZE2        5                       /* 5 bit base 2 exponent. */
 #define MAXFRACT2       ((1ul << MANTSIZE2) - 1) /* Maximum
fractional value. */
-#define MAXEXP2         ((1 <<EXPSIZE2) - 1)    /* Maximum exponent. */
+#define MAXEXP2         ((1 << EXPSIZE2) - 1)    /* Maximum exponent. */

 static comp2_t encode_comp2_t(u64 value)
 {
@@ -447,7 +447,7 @@ static comp2_t encode_comp2_t(u64 value)
 }
 #endif

-#if ACCT_VERSION=3
+#if ACCT_VERSION = 3
 /*
  * encode an u64 into a 32 bit IEEE float
  */
@@ -456,8 +456,9 @@ static u32 encode_float(u64 value)
 	unsigned exp = 190;
 	unsigned u;

-	if (value=0) return 0;
-	while ((s64)value > 0){
+	if (value = 0)
+		return 0;
+	while ((s64)value > 0) {
 		value <<= 1;
 		exp--;
 	}
@@ -513,13 +514,13 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 		       + current->group_leader->start_time.tv_nsec;
 	/* convert nsec -> AHZ */
 	elapsed = nsec_to_AHZ(run_time);
-#if ACCT_VERSION=3
+#if ACCT_VERSION = 3
 	ac.ac_etime = encode_float(elapsed);
 #else
 	ac.ac_etime = encode_comp_t(elapsed < (unsigned long) -1l ?
-	                       (unsigned long) elapsed : (unsigned long) -1l);
+				(unsigned long) elapsed:(unsigned long) -1l);
 #endif
-#if ACCT_VERSION=1 || ACCT_VERSION=2
+#if ACCT_VERSION = 1 || ACCT_VERSION = 2
 	{
 		/* new enlarged etime field */
 		comp2_t etime = encode_comp2_t(elapsed);
@@ -531,15 +532,15 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 	ac.ac_btime = get_seconds() - elapsed;
 	/* we really need to bite the bullet and change layout */
 	current_uid_gid(&ac.ac_uid, &ac.ac_gid);
-#if ACCT_VERSION=2
+#if ACCT_VERSION = 2
 	ac.ac_ahz = AHZ;
 #endif
-#if ACCT_VERSION=1 || ACCT_VERSION=2
+#if ACCT_VERSION = 1 || ACCT_VERSION = 2
 	/* backward-compatible 16 bit fields */
 	ac.ac_uid16 = ac.ac_uid;
 	ac.ac_gid16 = ac.ac_gid;
 #endif
-#if ACCT_VERSION=3
+#if ACCT_VERSION = 3
 	ac.ac_pid = task_tgid_nr_ns(current, ns);
 	rcu_read_lock();
 	ac.ac_ppid = task_tgid_nr_ns(rcu_dereference(current->real_parent), ns);
-- 
1.5.4.3

Thanks -
Manish

[PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Manish Katiyar]

Hi,

Below patch removes errors generated by checkpatch.pl in
kernel/utsname_sysctl.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/utsname_sysctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 3b34b35..d1dc4ca 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
write, struct file *filp,
 	int r;
 	memcpy(&uts_table, table, sizeof(uts_table));
 	uts_table.data = get_uts(table, write);
-	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
+	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
 	put_uts(table, write, uts_table.data);
 	return r;
 }
@@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
 	return 0;
 }

-__initcall(utsname_sysctl_init);
+device_initcall(utsname_sysctl_init);
-- 
1.5.4.3


Thanks -
Manish

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Manish Katiyar]

On Mon, Feb 16, 2009 at 12:17 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> Hi Ingo,
>>
>> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> Below patch removes errors generated by checkpatch.pl.
> When doing so use checkpatch only as a hint generator and do
> not concentrate only on the warnings/errors generated by checkpatch.
>
> Your patch is an improvement but please fix the remaining issues.
>
>        Sam

Thanks Sam, will address these and resend this patch.

Thanks -
Manish


>
> For example..
>> -     return kallsyms_token_table[ kallsyms_token_index[ kallsyms_names[off+1] ] ];
>> +     return kallsyms_token_table[kallsyms_token_index[kallsyms_names[off+1]]];
>
> Add spaces around [off+1] so it looks like this: [off + 1]
>
>
>>  }
>>
>>
>> @@ -133,13 +133,13 @@ static unsigned int get_symbol_offset(unsigned long pos)
>>
>>       /* use the closest marker we have. We have markers every 256 positions,
>>        * so that should be close enough */
>> -     name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
>> +     name = &kallsyms_names[kallsyms_markers[pos>>8]];
>
> Same here: [pos >> 8]
>
>
>>
>>       /* sequentially scan all the symbols up to the point we're searching for.
>>        * Every symbol is stored in a [<len>][<len> bytes of data] format, so we
>>        * just need to add the len to the current pointer for every symbol we
>>        * wish to skip */
>> -     for(i = 0; i < (pos&0xFF); i++)
>> +     for (i = 0; i < (pos&0xFF); i++)
> And here: (pos & 0xFF)
>
>>               name = name + (*name) + 1;
>>
>>       return name - kallsyms_names;
>> @@ -323,6 +323,7 @@ int sprint_symbol(char *buffer, unsigned long address)
>>
>>       return len;
>>  }
>> +EXPORT_SYMBOL_GPL(sprint_symbol);
>>
>>  /* Look up a kernel symbol and print it to the kernel messages. */
>>  void __print_symbol(const char *fmt, unsigned long address)
>> @@ -333,10 +334,10 @@ void __print_symbol(const char *fmt, unsigned
>> long address)
>>
>>       printk(fmt, buffer);
>>  }
>> +EXPORT_SYMBOL(__print_symbol);
>>
>>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
>> -struct kallsym_iter
>> -{
>> +struct kallsym_iter {
>>       loff_t pos;
>>       unsigned long value;
>>       unsigned int nameoff; /* If iterating in core kernel symbols */
>> @@ -385,7 +386,7 @@ static int update_iter(struct kallsym_iter *iter,
>> loff_t pos)
>>               iter->pos = pos;
>>               return get_ksymbol_mod(iter);
>>       }
>> -
>> +
>>       /* If we're not on the desired position, reset to new position. */
>>       if (pos != iter->pos)
>>               reset_iter(iter, pos);
>> @@ -420,7 +421,7 @@ static int s_show(struct seq_file *m, void *p)
>>  {
>>       struct kallsym_iter *iter = m->private;
>>
>> -     /* Some debugging symbols have no name.  Ignore them. */
>> +     /* Some debugging symbols have no name.  Ignore them. */
>>       if (!iter->name[0])
>>               return 0;
>>
>> @@ -432,11 +433,11 @@ static int s_show(struct seq_file *m, void *p)
>>               type = iter->exported ? toupper(iter->type) :
>>                                       tolower(iter->type);
>>               seq_printf(m, "%0*lx %c %s\t[%s]\n",
>> -                        (int)(2*sizeof(void*)),
>> +                        (int)(2*sizeof(void *)),
> (2 * sizeof(void *))
>
>
>>                          iter->value, type, iter->name, iter->module_name);
>>       } else
>>               seq_printf(m, "%0*lx %c %s\n",
>> -                        (int)(2*sizeof(void*)),
>> +                        (int)(2*sizeof(void *)),
> and here again.
>
>>                          iter->value, iter->type, iter->name);
>>       return 0;
>>  }
>> @@ -481,7 +482,5 @@ static int __init kallsyms_init(void)
>>       proc_create("kallsyms", 0444, NULL, &kallsyms_operations);
>>       return 0;
>>  }
>> -__initcall(kallsyms_init);
>> +device_initcall(kallsyms_init);
>>
>> -EXPORT_SYMBOL(__print_symbol);
>> -EXPORT_SYMBOL_GPL(sprint_symbol);
>
>

[PATCH] Remove errors caught by checkpatch.pl in kernel/signal.c

[Manish Katiyar]

Hi,

Below patch removes some errors generated by checkpatch.pl in
kernel/signal.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/signal.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index e737597..9fde101 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t
*signal, sigset_t *blocked)
 	switch (_NSIG_WORDS) {
 	default:
 		for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;)
-			ready |= signal->sig[i] &~ blocked->sig[i];
+			ready |= signal->sig[i] & ~blocked->sig[i];
 		break;

-	case 4: ready  = signal->sig[3] &~ blocked->sig[3];
-		ready |= signal->sig[2] &~ blocked->sig[2];
-		ready |= signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 4: ready  = signal->sig[3] & ~blocked->sig[3];
+		ready |= signal->sig[2] & ~blocked->sig[2];
+		ready |= signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;

-	case 2: ready  = signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 2: ready  = signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;

-	case 1: ready  = signal->sig[0] &~ blocked->sig[0];
+	case 1: ready  = signal->sig[0] & ~blocked->sig[0];
 	}
 	return ready !=	0;
 }
@@ -156,22 +156,22 @@ int next_signal(struct sigpending *pending,
sigset_t *mask)
 	switch (_NSIG_WORDS) {
 	default:
 		for (i = 0; i < _NSIG_WORDS; ++i, ++s, ++m)
-			if ((x = *s &~ *m) != 0) {
+			if ((x = *s & ~*m) != 0) {
 				sig = ffz(~x) + i*_NSIG_BPW + 1;
 				break;
 			}
 		break;

-	case 2: if ((x = s[0] &~ m[0]) != 0)
+	case 2: if ((x = s[0] & ~m[0]) != 0)
 			sig = 1;
-		else if ((x = s[1] &~ m[1]) != 0)
+		else if ((x = s[1] & ~m[1]) != 0)
 			sig = _NSIG_BPW + 1;
 		else
 			break;
 		sig += ffz(~x);
 		break;

-	case 1: if ((x = *s &~ *m) != 0)
+	case 1: if ((x = *s & ~*m) != 0)
 			sig = ffz(~x) + 1;
 		break;
 	}
@@ -414,7 +414,7 @@ static int __dequeue_signal(struct sigpending
*pending, sigset_t *mask,
 }

 /*
- * Dequeue a signal and return the element to the caller, which is
+ * Dequeue a signal and return the element to the caller, which is
  * expected to free it.
  *
  * All callers have to hold the siglock.
@@ -914,7 +914,7 @@ static void print_fatal_signal(struct pt_regs
*regs, int signr)

 static int __init setup_print_fatal_signals(char *str)
 {
-	get_option (&str, &print_fatal_signals);
+	get_option(&str, &print_fatal_signals);

 	return 1;
 }
@@ -1164,7 +1164,7 @@ static int kill_something_info(int sig, struct
siginfo *info, pid_t pid)
 				pid ? find_vpid(-pid) : task_pgrp(current));
 	} else {
 		int retval = 0, count = 0;
-		struct task_struct * p;
+		struct task_struct *p;

 		for_each_process(p) {
 			if (task_pid_vnr(p) > 1 &&
@@ -1264,19 +1264,18 @@ EXPORT_SYMBOL(kill_pid);
  * These functions support sending signals using preallocated sigqueue
  * structures.  This is needed "because realtime applications cannot
  * afford to lose notifications of asynchronous events, like timer
- * expirations or I/O completions".  In the case of Posix Timers
+ * expirations or I/O completions".  In the case of Posix Timers
  * we allocate the sigqueue structure from the timer_create.  If this
  * allocation fails we are able to report the failure to the application
  * with an EAGAIN error.
  */
-
 struct sigqueue *sigqueue_alloc(void)
 {
 	struct sigqueue *q;

 	if ((q = __sigqueue_alloc(current, GFP_KERNEL, 0)))
 		q->flags |= SIGQUEUE_PREALLOC;
-	return(q);
+	return q;
 }

 void sigqueue_free(struct sigqueue *q)
@@ -1374,7 +1373,7 @@ int do_notify_parent(struct task_struct *tsk, int sig)
  	BUG_ON(task_is_stopped_or_traced(tsk));

 	BUG_ON(!tsk->ptrace &&
-	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
+		(tsk->group_leader != tsk || !thread_group_empty(tsk)));

 	info.si_signo = sig;
 	info.si_errno = 0;
@@ -2072,7 +2071,7 @@ long do_sigpending(void __user *set, unsigned
long sigsetsize)

 out:
 	return error;
-}	
+}

 SYSCALL_DEFINE2(rt_sigpending, sigset_t __user *, set, size_t, sigsetsize)
 {
@@ -2085,7 +2084,7 @@ int copy_siginfo_to_user(siginfo_t __user *to,
siginfo_t *from)
 {
 	int err;

-	if (!access_ok (VERIFY_WRITE, to, sizeof(siginfo_t)))
+	if (!access_ok(VERIFY_WRITE, to, sizeof(siginfo_t)))
 		return -EFAULT;
 	if (from->si_code < 0)
 		return __copy_to_user(to, from, sizeof(siginfo_t))
@@ -2161,7 +2160,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const sigset_t
__user *, uthese,

 	if (copy_from_user(&these, uthese, sizeof(these)))
 		return -EFAULT;
-		
+
 	/*
 	 * Invert the set of allowed signals to get those we
 	 * want to block.
@@ -2364,7 +2363,7 @@ int do_sigaction(int sig, struct k_sigaction
*act, struct k_sigaction *oact)
 }

 int
-do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss,
unsigned long sp)
+do_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
unsigned long sp)
 {
 	stack_t oss;
 	int error;
@@ -2479,7 +2478,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how,
old_sigset_t __user *, set,
 			goto set_old;
 	} else if (oset) {
 		old_set = current->blocked.sig[0];
-	set_old:
+set_old:
 		error = -EFAULT;
 		if (copy_to_user(oset, &old_set, sizeof(*oset)))
 			goto out;
-- 
1.5.4.3


Thanks -
Manish

[PATCH] Remove errors caught by checkpatch.pl in kernel/signal.c

[Manish Katiyar]

Hi,

Below patch removes some errors generated by checkpatch.pl in
kernel/signal.c. Caught by Ingo's code-quality script.


Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/signal.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index e737597..9fde101 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t
*signal, sigset_t *blocked)
 	switch (_NSIG_WORDS) {
 	default:
 		for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;)
-			ready |= signal->sig[i] &~ blocked->sig[i];
+			ready |= signal->sig[i] & ~blocked->sig[i];
 		break;

-	case 4: ready  = signal->sig[3] &~ blocked->sig[3];
-		ready |= signal->sig[2] &~ blocked->sig[2];
-		ready |= signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 4: ready  = signal->sig[3] & ~blocked->sig[3];
+		ready |= signal->sig[2] & ~blocked->sig[2];
+		ready |= signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;

-	case 2: ready  = signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 2: ready  = signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;

-	case 1: ready  = signal->sig[0] &~ blocked->sig[0];
+	case 1: ready  = signal->sig[0] & ~blocked->sig[0];
 	}
 	return ready !=	0;
 }
@@ -156,22 +156,22 @@ int next_signal(struct sigpending *pending,
sigset_t *mask)
 	switch (_NSIG_WORDS) {
 	default:
 		for (i = 0; i < _NSIG_WORDS; ++i, ++s, ++m)
-			if ((x = *s &~ *m) != 0) {
+			if ((x = *s & ~*m) != 0) {
 				sig = ffz(~x) + i*_NSIG_BPW + 1;
 				break;
 			}
 		break;

-	case 2: if ((x = s[0] &~ m[0]) != 0)
+	case 2: if ((x = s[0] & ~m[0]) != 0)
 			sig = 1;
-		else if ((x = s[1] &~ m[1]) != 0)
+		else if ((x = s[1] & ~m[1]) != 0)
 			sig = _NSIG_BPW + 1;
 		else
 			break;
 		sig += ffz(~x);
 		break;

-	case 1: if ((x = *s &~ *m) != 0)
+	case 1: if ((x = *s & ~*m) != 0)
 			sig = ffz(~x) + 1;
 		break;
 	}
@@ -414,7 +414,7 @@ static int __dequeue_signal(struct sigpending
*pending, sigset_t *mask,
 }

 /*
- * Dequeue a signal and return the element to the caller, which is
+ * Dequeue a signal and return the element to the caller, which is
  * expected to free it.
  *
  * All callers have to hold the siglock.
@@ -914,7 +914,7 @@ static void print_fatal_signal(struct pt_regs
*regs, int signr)

 static int __init setup_print_fatal_signals(char *str)
 {
-	get_option (&str, &print_fatal_signals);
+	get_option(&str, &print_fatal_signals);

 	return 1;
 }
@@ -1164,7 +1164,7 @@ static int kill_something_info(int sig, struct
siginfo *info, pid_t pid)
 				pid ? find_vpid(-pid) : task_pgrp(current));
 	} else {
 		int retval = 0, count = 0;
-		struct task_struct * p;
+		struct task_struct *p;

 		for_each_process(p) {
 			if (task_pid_vnr(p) > 1 &&
@@ -1264,19 +1264,18 @@ EXPORT_SYMBOL(kill_pid);
  * These functions support sending signals using preallocated sigqueue
  * structures.  This is needed "because realtime applications cannot
  * afford to lose notifications of asynchronous events, like timer
- * expirations or I/O completions".  In the case of Posix Timers
+ * expirations or I/O completions".  In the case of Posix Timers
  * we allocate the sigqueue structure from the timer_create.  If this
  * allocation fails we are able to report the failure to the application
  * with an EAGAIN error.
  */
-
 struct sigqueue *sigqueue_alloc(void)
 {
 	struct sigqueue *q;

 	if ((q = __sigqueue_alloc(current, GFP_KERNEL, 0)))
 		q->flags |= SIGQUEUE_PREALLOC;
-	return(q);
+	return q;
 }

 void sigqueue_free(struct sigqueue *q)
@@ -1374,7 +1373,7 @@ int do_notify_parent(struct task_struct *tsk, int sig)
  	BUG_ON(task_is_stopped_or_traced(tsk));

 	BUG_ON(!tsk->ptrace &&
-	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
+		(tsk->group_leader != tsk || !thread_group_empty(tsk)));

 	info.si_signo = sig;
 	info.si_errno = 0;
@@ -2072,7 +2071,7 @@ long do_sigpending(void __user *set, unsigned
long sigsetsize)

 out:
 	return error;
-}	
+}

 SYSCALL_DEFINE2(rt_sigpending, sigset_t __user *, set, size_t, sigsetsize)
 {
@@ -2085,7 +2084,7 @@ int copy_siginfo_to_user(siginfo_t __user *to,
siginfo_t *from)
 {
 	int err;

-	if (!access_ok (VERIFY_WRITE, to, sizeof(siginfo_t)))
+	if (!access_ok(VERIFY_WRITE, to, sizeof(siginfo_t)))
 		return -EFAULT;
 	if (from->si_code < 0)
 		return __copy_to_user(to, from, sizeof(siginfo_t))
@@ -2161,7 +2160,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const sigset_t
__user *, uthese,

 	if (copy_from_user(&these, uthese, sizeof(these)))
 		return -EFAULT;
-		
+
 	/*
 	 * Invert the set of allowed signals to get those we
 	 * want to block.
@@ -2364,7 +2363,7 @@ int do_sigaction(int sig, struct k_sigaction
*act, struct k_sigaction *oact)
 }

 int
-do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss,
unsigned long sp)
+do_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
unsigned long sp)
 {
 	stack_t oss;
 	int error;
@@ -2479,7 +2478,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how,
old_sigset_t __user *, set,
 			goto set_old;
 	} else if (oset) {
 		old_set = current->blocked.sig[0];
-	set_old:
+set_old:
 		error = -EFAULT;
 		if (copy_to_user(oset, &old_set, sizeof(*oset)))
 			goto out;
-- 
1.5.4.3


Thanks -
Manish

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/signal.c

[Ingo Molnar]

* Manish Katiyar <mkatiyar@gmail.com> wrote:

> Hi,
> 
> Below patch removes some errors generated by checkpatch.pl in
> kernel/signal.c. Caught by Ingo's code-quality script.
> 
> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  kernel/signal.c |   47 +++++++++++++++++++++++------------------------
>  1 files changed, 23 insertions(+), 24 deletions(-)

Note, your patch has line-wrap problems, such as:

> +++ b/kernel/signal.c
> @@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t
> *signal, sigset_t *blocked)
>  	switch (_NSIG_WORDS) {

Causing:

 patch: **** malformed patch at line 23: *signal, sigset_t *blocked)

See Documentation/email-clients.txt.

Also, kernel/signal.c needs a thorough cleanup, and your patch only
handles about a third of the errors+warnings:

 before: total: 61 errors, 28 warnings, 3 checks, 2615 lines checked
  after: total: 30 errors, 24 warnings, 3 checks, 2614 lines checked

If then it's best to bring the count down very close to zero (so
that only the obvious checkpatch false positives are left), and also
have a really good human-coder look at signal.c's:

 - structure and code flow
 - variable naming
 - include files section
 - general splitup and function ordering
 - comment style consistency

etc. - to turn it into a really modern, nice-to-look-at and hackable
Linux kernel file.

The checkpatch motivated cleanups really only tell us half of the
story that is usually in such a long-forgotten file - and by fixing
the checkpatch warnings only it gives us a false impression of
cleanliness. So the checkpatch fixes should go hand in hand with
more grounds-up cleanups. (which can all still stay at the pure
style level, so that the .o output does not change - for easy
verification.)

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Manish Katiyar <mkatiyar@gmail.com> wrote:

> Hi,
> 
> Below patch removes some errors generated by checkpatch.pl in
> kernel/signal.c. Caught by Ingo's code-quality script.
> 
> 
> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
> ---
>  kernel/signal.c |   47 +++++++++++++++++++++++------------------------
>  1 files changed, 23 insertions(+), 24 deletions(-)

Note, your patch has line-wrap problems, such as:

> +++ b/kernel/signal.c
> @@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t
> *signal, sigset_t *blocked)
>  	switch (_NSIG_WORDS) {

Causing:

 patch: **** malformed patch at line 23: *signal, sigset_t *blocked)

See Documentation/email-clients.txt.

Also, kernel/signal.c needs a thorough cleanup, and your patch only
handles about a third of the errors+warnings:

 before: total: 61 errors, 28 warnings, 3 checks, 2615 lines checked
  after: total: 30 errors, 24 warnings, 3 checks, 2614 lines checked

If then it's best to bring the count down very close to zero (so
that only the obvious checkpatch false positives are left), and also
have a really good human-coder look at signal.c's:

 - structure and code flow
 - variable naming
 - include files section
 - general splitup and function ordering
 - comment style consistency

etc. - to turn it into a really modern, nice-to-look-at and hackable
Linux kernel file.

The checkpatch motivated cleanups really only tell us half of the
story that is usually in such a long-forgotten file - and by fixing
the checkpatch warnings only it gives us a false impression of
cleanliness. So the checkpatch fixes should go hand in hand with
more grounds-up cleanups. (which can all still stay at the pure
style level, so that the .o output does not change - for easy
verification.)

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> Hi Ingo,
>> 
>> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> Below patch removes errors generated by checkpatch.pl.
> When doing so use checkpatch only as a hint generator and do
> not concentrate only on the warnings/errors generated by checkpatch.
> 
> Your patch is an improvement but please fix the remaining issues.

Furthermore, the changelog is bad (non-exiting in fact).

The fact that the issues where discovered using checkpatch is absolutely
uninteresting.  The changelog should describe /what/ is fixed, e.g.
whitespace, maybe other things.  (In case of nontrivial changes the log
may also need to explain not only the /what but also the /how/, but this
does not apply to patches like this one.)
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> Hi Ingo,
>> 
>> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> Below patch removes errors generated by checkpatch.pl.
> When doing so use checkpatch only as a hint generator and do
> not concentrate only on the warnings/errors generated by checkpatch.
> 
> Your patch is an improvement but please fix the remaining issues.

Furthermore, the changelog is bad (non-exiting in fact).

The fact that the issues where discovered using checkpatch is absolutely
uninteresting.  The changelog should describe /what/ is fixed, e.g.
whitespace, maybe other things.  (In case of nontrivial changes the log
may also need to explain not only the /what but also the /how/, but this
does not apply to patches like this one.)
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Stefan Richter]

Manish Katiyar wrote:
> --- a/kernel/utsname_sysctl.c
> +++ b/kernel/utsname_sysctl.c
> @@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
> write, struct file *filp,
>  	int r;
>  	memcpy(&uts_table, table, sizeof(uts_table));
>  	uts_table.data = get_uts(table, write);
> -	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
> +	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
>  	put_uts(table, write, uts_table.data);
>  	return r;
>  }
> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>  	return 0;
>  }
> 
> -__initcall(utsname_sysctl_init);
> +device_initcall(utsname_sysctl_init);

The changelog should note that this is a whitespace fix + initcall
annotation change.

That checkpatch was somehow involved in the creation of this patch is
entirely uninteresting for the changelog.
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Stefan Richter]

Manish Katiyar wrote:
> --- a/kernel/utsname_sysctl.c
> +++ b/kernel/utsname_sysctl.c
> @@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
> write, struct file *filp,
>  	int r;
>  	memcpy(&uts_table, table, sizeof(uts_table));
>  	uts_table.data = get_uts(table, write);
> -	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
> +	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
>  	put_uts(table, write, uts_table.data);
>  	return r;
>  }
> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>  	return 0;
>  }
> 
> -__initcall(utsname_sysctl_init);
> +device_initcall(utsname_sysctl_init);

The changelog should note that this is a whitespace fix + initcall
annotation change.

That checkpatch was somehow involved in the creation of this patch is
entirely uninteresting for the changelog.
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/acct.c

[Stefan Richter]

Manish Katiyar wrote:
> Below patch removes errors generated by checkpatch.pl in
> kernel/acct.c. Caught by Ingo's code-quality script.

Bad changelog:  Just say that this is a whitespace adjustment.

> @@ -416,7 +416,7 @@ static comp_t encode_comp_t(unsigned long value)
>  #define MANTSIZE2       20                      /* 20 bit mantissa. */
>  #define EXPSIZE2        5                       /* 5 bit base 2 exponent. */
>  #define MAXFRACT2       ((1ul << MANTSIZE2) - 1) /* Maximum
> fractional value. */

Your MUA inserted line wraps where there should none.  This won't apply
as a patch anymore.

> @@ -513,13 +514,13 @@ static void do_acct_process(struct bsd_acct_struct *acct,
>  		       + current->group_leader->start_time.tv_nsec;
>  	/* convert nsec -> AHZ */
>  	elapsed = nsec_to_AHZ(run_time);
> -#if ACCT_VERSION==3
> +#if ACCT_VERSION == 3
>  	ac.ac_etime = encode_float(elapsed);
>  #else
>  	ac.ac_etime = encode_comp_t(elapsed < (unsigned long) -1l ?
> -	                       (unsigned long) elapsed : (unsigned long) -1l);
> +				(unsigned long) elapsed:(unsigned long) -1l);

Should IMO be	(unsigned long)elapsed : (unsigned long)-1l);

or simply stay as it was.

But more importantly, the whole expression looks bogus to me.  elapsed
is an u64 and thus clearly defined regarding its size, while unsigned
long will have different sizes on different CPU architectures.

Is this intentional?
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/acct.c

[Stefan Richter]

Manish Katiyar wrote:
> Below patch removes errors generated by checkpatch.pl in
> kernel/acct.c. Caught by Ingo's code-quality script.

Bad changelog:  Just say that this is a whitespace adjustment.

> @@ -416,7 +416,7 @@ static comp_t encode_comp_t(unsigned long value)
>  #define MANTSIZE2       20                      /* 20 bit mantissa. */
>  #define EXPSIZE2        5                       /* 5 bit base 2 exponent. */
>  #define MAXFRACT2       ((1ul << MANTSIZE2) - 1) /* Maximum
> fractional value. */

Your MUA inserted line wraps where there should none.  This won't apply
as a patch anymore.

> @@ -513,13 +514,13 @@ static void do_acct_process(struct bsd_acct_struct *acct,
>  		       + current->group_leader->start_time.tv_nsec;
>  	/* convert nsec -> AHZ */
>  	elapsed = nsec_to_AHZ(run_time);
> -#if ACCT_VERSION=3
> +#if ACCT_VERSION = 3
>  	ac.ac_etime = encode_float(elapsed);
>  #else
>  	ac.ac_etime = encode_comp_t(elapsed < (unsigned long) -1l ?
> -	                       (unsigned long) elapsed : (unsigned long) -1l);
> +				(unsigned long) elapsed:(unsigned long) -1l);

Should IMO be	(unsigned long)elapsed : (unsigned long)-1l);

or simply stay as it was.

But more importantly, the whole expression looks bogus to me.  elapsed
is an u64 and thus clearly defined regarding its size, while unsigned
long will have different sizes on different CPU architectures.

Is this intentional?
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> >> Hi Ingo,
> >> 
> >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> >> Below patch removes errors generated by checkpatch.pl.
> > When doing so use checkpatch only as a hint generator and do
> > not concentrate only on the warnings/errors generated by checkpatch.
> > 
> > Your patch is an improvement but please fix the remaining issues.
> 
> Furthermore, the changelog is bad (non-exiting in fact).
> 
> The fact that the issues where discovered using checkpatch is absolutely
> uninteresting.  The changelog should describe /what/ is fixed, e.g.
> whitespace, maybe other things.  (In case of nontrivial changes the log
> may also need to explain not only the /what but also the /how/, but this
> does not apply to patches like this one.)

The commit log definitely needs enhancements but it's not uninteresting 
at all what tools were used to arrive to a change. It shouldnt be in the 
title, but can be mentioned in the changelog itself. (and should be 
mentioned if the cleanup ever gets as far as the mainline kernel - if a 
good and acceptable commit results out of a tool's usage then that tool 
needs to be advertised some more.)

Nevertheless i think that these cleanups need to go a bit wider than 
just the issues pointed, as i described it in the related 
kernel/signal.c thread.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> >> Hi Ingo,
> >> 
> >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> >> Below patch removes errors generated by checkpatch.pl.
> > When doing so use checkpatch only as a hint generator and do
> > not concentrate only on the warnings/errors generated by checkpatch.
> > 
> > Your patch is an improvement but please fix the remaining issues.
> 
> Furthermore, the changelog is bad (non-exiting in fact).
> 
> The fact that the issues where discovered using checkpatch is absolutely
> uninteresting.  The changelog should describe /what/ is fixed, e.g.
> whitespace, maybe other things.  (In case of nontrivial changes the log
> may also need to explain not only the /what but also the /how/, but this
> does not apply to patches like this one.)

The commit log definitely needs enhancements but it's not uninteresting 
at all what tools were used to arrive to a change. It shouldnt be in the 
title, but can be mentioned in the changelog itself. (and should be 
mentioned if the cleanup ever gets as far as the mainline kernel - if a 
good and acceptable commit results out of a tool's usage then that tool 
needs to be advertised some more.)

Nevertheless i think that these cleanups need to go a bit wider than 
just the issues pointed, as i described it in the related 
kernel/signal.c thread.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> Furthermore, the changelog is bad (non-exiting in fact).
>> 
>> The fact that the issues where discovered using checkpatch is absolutely
>> uninteresting.  The changelog should describe /what/ is fixed, [...]
> 
> The commit log definitely needs enhancements but it's not uninteresting 
> at all what tools were used to arrive to a change. [...] if a
> good and acceptable commit results out of a tool's usage then that tool 
> needs to be advertised some more.)

Fine, then the author could mention it below the --- delimitor in the
patch posting.  The changelog however, as annotation of the source
history, is not a billboard.  We also don't describe for example that a
nice cup of hot Earl Grey or whatever was vital to the creation of a patch.
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> Furthermore, the changelog is bad (non-exiting in fact).
>> 
>> The fact that the issues where discovered using checkpatch is absolutely
>> uninteresting.  The changelog should describe /what/ is fixed, [...]
> 
> The commit log definitely needs enhancements but it's not uninteresting 
> at all what tools were used to arrive to a change. [...] if a
> good and acceptable commit results out of a tool's usage then that tool 
> needs to be advertised some more.)

Fine, then the author could mention it below the --- delimitor in the
patch posting.  The changelog however, as annotation of the source
history, is not a billboard.  We also don't describe for example that a
nice cup of hot Earl Grey or whatever was vital to the creation of a patch.
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> Furthermore, the changelog is bad (non-exiting in fact).
> >> 
> >> The fact that the issues where discovered using checkpatch is absolutely
> >> uninteresting.  The changelog should describe /what/ is fixed, [...]
> > 
> > The commit log definitely needs enhancements but it's not uninteresting 
> > at all what tools were used to arrive to a change. [...] if a
> > good and acceptable commit results out of a tool's usage then that tool 
> > needs to be advertised some more.)
> 
> Fine, then the author could mention it below the --- delimitor in the 
> patch posting.  The changelog however, as annotation of the source 
> history, is not a billboard.  We also don't describe for example that 
> a nice cup of hot Earl Grey or whatever was vital to the creation of a 
> patch.

Well there's a difference between a nice cup of tea (that really has no 
direct connection to kernel development) and a tool that is in the Linux 
kernel specifically for the purpose of helping keep code clean, and that 
was used to come up with a cleanup.

We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
ftrace, kmemcheck and other tools as well when it motives to fix a bug 
or uncleanliness. We routinely mention checkpatch as well when it 
catches an uncleanliness in a submitted patch. It is absolutely fine to 
mention checkpatch when it catches uncleanliness in code that already 
got merged. I dont understand your point.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> Furthermore, the changelog is bad (non-exiting in fact).
> >> 
> >> The fact that the issues where discovered using checkpatch is absolutely
> >> uninteresting.  The changelog should describe /what/ is fixed, [...]
> > 
> > The commit log definitely needs enhancements but it's not uninteresting 
> > at all what tools were used to arrive to a change. [...] if a
> > good and acceptable commit results out of a tool's usage then that tool 
> > needs to be advertised some more.)
> 
> Fine, then the author could mention it below the --- delimitor in the 
> patch posting.  The changelog however, as annotation of the source 
> history, is not a billboard.  We also don't describe for example that 
> a nice cup of hot Earl Grey or whatever was vital to the creation of a 
> patch.

Well there's a difference between a nice cup of tea (that really has no 
direct connection to kernel development) and a tool that is in the Linux 
kernel specifically for the purpose of helping keep code clean, and that 
was used to come up with a cleanup.

We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
ftrace, kmemcheck and other tools as well when it motives to fix a bug 
or uncleanliness. We routinely mention checkpatch as well when it 
catches an uncleanliness in a submitted patch. It is absolutely fine to 
mention checkpatch when it catches uncleanliness in code that already 
got merged. I dont understand your point.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Paolo Ciarrocchi]

On Mon, Feb 16, 2009 at 2:28 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>
>> On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
>> > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> >> Hi Ingo,
>> >>
>> >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> >> Below patch removes errors generated by checkpatch.pl.
>> > When doing so use checkpatch only as a hint generator and do
>> > not concentrate only on the warnings/errors generated by checkpatch.
>> >
>> > Your patch is an improvement but please fix the remaining issues.
>>
>> Furthermore, the changelog is bad (non-exiting in fact).
>>
>> The fact that the issues where discovered using checkpatch is absolutely
>> uninteresting.  The changelog should describe /what/ is fixed, e.g.
>> whitespace, maybe other things.  (In case of nontrivial changes the log
>> may also need to explain not only the /what but also the /how/, but this
>> does not apply to patches like this one.)
>
> The commit log definitely needs enhancements but it's not uninteresting
> at all what tools were used to arrive to a change. It shouldnt be in the
> title, but can be mentioned in the changelog itself. (and should be
> mentioned if the cleanup ever gets as far as the mainline kernel - if a
> good and acceptable commit results out of a tool's usage then that tool
> needs to be advertised some more.)

I think it's a good idea to add some information to the changelog
explaining that the patch is not going to modify the gcc binary
output.
Something like:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=20211e4d344729f4d4c93da37a590fc1c3a1fd9b


Ciao,
-- 
Paolo
http://paolo.ciarrocchi.googlepages.com/
http://mypage.vodafone.it/

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Paolo Ciarrocchi]

On Mon, Feb 16, 2009 at 2:28 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>
>> On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
>> > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
>> >> Hi Ingo,
>> >>
>> >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
>> >> Below patch removes errors generated by checkpatch.pl.
>> > When doing so use checkpatch only as a hint generator and do
>> > not concentrate only on the warnings/errors generated by checkpatch.
>> >
>> > Your patch is an improvement but please fix the remaining issues.
>>
>> Furthermore, the changelog is bad (non-exiting in fact).
>>
>> The fact that the issues where discovered using checkpatch is absolutely
>> uninteresting.  The changelog should describe /what/ is fixed, e.g.
>> whitespace, maybe other things.  (In case of nontrivial changes the log
>> may also need to explain not only the /what but also the /how/, but this
>> does not apply to patches like this one.)
>
> The commit log definitely needs enhancements but it's not uninteresting
> at all what tools were used to arrive to a change. It shouldnt be in the
> title, but can be mentioned in the changelog itself. (and should be
> mentioned if the cleanup ever gets as far as the mainline kernel - if a
> good and acceptable commit results out of a tool's usage then that tool
> needs to be advertised some more.)

I think it's a good idea to add some information to the changelog
explaining that the patch is not going to modify the gcc binary
output.
Something like:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h 211e4d344729f4d4c93da37a590fc1c3a1fd9b


Ciao,
-- 
Paolo
http://paolo.ciarrocchi.googlepages.com/
http://mypage.vodafone.it/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Manish Katiyar]

[resending with changed changelog based on stefan's comments]

On Mon, Feb 16, 2009 at 6:39 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Manish Katiyar wrote:
>> --- a/kernel/utsname_sysctl.c
>> +++ b/kernel/utsname_sysctl.c
>> @@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
>> write, struct file *filp,
>>       int r;
>>       memcpy(&uts_table, table, sizeof(uts_table));
>>       uts_table.data = get_uts(table, write);
>> -     r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
>> +     r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
>>       put_uts(table, write, uts_table.data);
>>       return r;
>>  }
>> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>>       return 0;
>>  }
>>
>> -__initcall(utsname_sysctl_init);
>> +device_initcall(utsname_sysctl_init);
>
> The changelog should note that this is a whitespace fix + initcall
> annotation change.
>
> That checkpatch was somehow involved in the creation of this patch is
> entirely uninteresting for the changelog.

Replace deprecated __initcall with equivalent device_initcall. Also
fix whitespaces.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/utsname_sysctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 3b34b35..d1dc4ca 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
write, struct file *filp,
 	int r;
 	memcpy(&uts_table, table, sizeof(uts_table));
 	uts_table.data = get_uts(table, write);
-	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
+	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
 	put_uts(table, write, uts_table.data);
 	return r;
 }
@@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
 	return 0;
 }

-__initcall(utsname_sysctl_init);
+device_initcall(utsname_sysctl_init);
-- 
1.5.4.3


Thanks -
Manish

> --
> Stefan Richter
> -=====-==--= --=- =----
> http://arcgraph.de/sr/
>

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Manish Katiyar]

[resending with changed changelog based on stefan's comments]

On Mon, Feb 16, 2009 at 6:39 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Manish Katiyar wrote:
>> --- a/kernel/utsname_sysctl.c
>> +++ b/kernel/utsname_sysctl.c
>> @@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
>> write, struct file *filp,
>>       int r;
>>       memcpy(&uts_table, table, sizeof(uts_table));
>>       uts_table.data = get_uts(table, write);
>> -     r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
>> +     r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
>>       put_uts(table, write, uts_table.data);
>>       return r;
>>  }
>> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>>       return 0;
>>  }
>>
>> -__initcall(utsname_sysctl_init);
>> +device_initcall(utsname_sysctl_init);
>
> The changelog should note that this is a whitespace fix + initcall
> annotation change.
>
> That checkpatch was somehow involved in the creation of this patch is
> entirely uninteresting for the changelog.

Replace deprecated __initcall with equivalent device_initcall. Also
fix whitespaces.

Signed-off-by: Manish Katiyar <mkatiyar@gmail.com>
---
 kernel/utsname_sysctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 3b34b35..d1dc4ca 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -49,7 +49,7 @@ static int proc_do_uts_string(ctl_table *table, int
write, struct file *filp,
 	int r;
 	memcpy(&uts_table, table, sizeof(uts_table));
 	uts_table.data = get_uts(table, write);
-	r = proc_dostring(&uts_table,write,filp,buffer,lenp, ppos);
+	r = proc_dostring(&uts_table, write, filp, buffer, lenp, ppos);
 	put_uts(table, write, uts_table.data);
 	return r;
 }
@@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
 	return 0;
 }

-__initcall(utsname_sysctl_init);
+device_initcall(utsname_sysctl_init);
-- 
1.5.4.3


Thanks -
Manish

> --
> Stefan Richter
> -===-=--= --=- =----
> http://arcgraph.de/sr/
>

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> or uncleanliness. [...] It is absolutely fine to
> mention checkpatch when it catches uncleanliness in code that already 
> got merged. I dont understand your point.

I wrote "don't mention checkpatch" but I really meant "think about what
the effect of the patch is and describe this".

It's not really a hard problem to mention checkpatch --- it is a problem
to make it the main point or, like in this case, the only point of the
changelog.  (Furthermore, it is also a problem to do something routinely
*if* doing it does not make sense.  There routinely appear coccinelle
metapatch sources in changelogs.  That does not make sense at all, and
doing it routinely is not a justification in itself.)

So, "don't mention checkpatch" is simply a rule of thumb; read it as "I
mentioned checkpatch in the changelog --- wait, I have possibly written
a changelog that is besides the point; I should think about it once
more".  :-)

Now, when this particular patch is updated to get a good changelog, then
the title could become e.g.:
	kernel/kallsyms: change initcall level; adjust whitespace
and anything more than that is just fluff and wasted electrons. Actually
the changelog should rather contain a note on why device_initcall is
supposed to be the correct initcall level.

Fixes due to reports from sparse, lockdep, coverity, coccinelle, etc.
are the in this respect the same as fixes due to reports from
checkpatch:  Patch titles should for example be
  - "fix potential deadlock..."
  - "fix use-after free..."
  - "use XYZ helper..."
  - "adjust whitespace..."
and *not* something like "fix lockdep backtrace" or whatever.

A difference would be a patch title like "add sparse annotations"
because this is indeed about what the patch does, not by which means it
was created.

Why do I make a fuzz?  Well, because many of our changelogs really suck
and we need to become better in general.
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> or uncleanliness. [...] It is absolutely fine to
> mention checkpatch when it catches uncleanliness in code that already 
> got merged. I dont understand your point.

I wrote "don't mention checkpatch" but I really meant "think about what
the effect of the patch is and describe this".

It's not really a hard problem to mention checkpatch --- it is a problem
to make it the main point or, like in this case, the only point of the
changelog.  (Furthermore, it is also a problem to do something routinely
*if* doing it does not make sense.  There routinely appear coccinelle
metapatch sources in changelogs.  That does not make sense at all, and
doing it routinely is not a justification in itself.)

So, "don't mention checkpatch" is simply a rule of thumb; read it as "I
mentioned checkpatch in the changelog --- wait, I have possibly written
a changelog that is besides the point; I should think about it once
more".  :-)

Now, when this particular patch is updated to get a good changelog, then
the title could become e.g.:
	kernel/kallsyms: change initcall level; adjust whitespace
and anything more than that is just fluff and wasted electrons. Actually
the changelog should rather contain a note on why device_initcall is
supposed to be the correct initcall level.

Fixes due to reports from sparse, lockdep, coverity, coccinelle, etc.
are the in this respect the same as fixes due to reports from
checkpatch:  Patch titles should for example be
  - "fix potential deadlock..."
  - "fix use-after free..."
  - "use XYZ helper..."
  - "adjust whitespace..."
and *not* something like "fix lockdep backtrace" or whatever.

A difference would be a patch title like "add sparse annotations"
because this is indeed about what the patch does, not by which means it
was created.

Why do I make a fuzz?  Well, because many of our changelogs really suck
and we need to become better in general.
-- 
Stefan Richter
-===-=-== -=-= -=-http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Stefan Richter]

Manish Katiyar wrote:
> On Mon, Feb 16, 2009 at 6:39 PM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
>> The changelog should note that this is a whitespace fix + initcall
>> annotation change.

Sorry, I didn't get the point of __initcall/device_initcall.  It's not
an annotation; it defines a call sequence level.

> Replace deprecated __initcall with equivalent device_initcall. Also
> fix whitespaces.
...
> --- a/kernel/utsname_sysctl.c
> +++ b/kernel/utsname_sysctl.c
...
> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>  	return 0;
>  }
> 
> -__initcall(utsname_sysctl_init);
> +device_initcall(utsname_sysctl_init);

It is equivalent, but is it also the most appropriate one?
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/utsname_sysctl.c

[Stefan Richter]

Manish Katiyar wrote:
> On Mon, Feb 16, 2009 at 6:39 PM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
>> The changelog should note that this is a whitespace fix + initcall
>> annotation change.

Sorry, I didn't get the point of __initcall/device_initcall.  It's not
an annotation; it defines a call sequence level.

> Replace deprecated __initcall with equivalent device_initcall. Also
> fix whitespaces.
...
> --- a/kernel/utsname_sysctl.c
> +++ b/kernel/utsname_sysctl.c
...
> @@ -142,4 +142,4 @@ static int __init utsname_sysctl_init(void)
>  	return 0;
>  }
> 
> -__initcall(utsname_sysctl_init);
> +device_initcall(utsname_sysctl_init);

It is equivalent, but is it also the most appropriate one?
-- 
Stefan Richter
-===-=-== -=-= -=-http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Manish Katiyar]

On Mon, Feb 16, 2009 at 8:52 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Ingo Molnar wrote:
>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak,
>> ftrace, kmemcheck and other tools as well when it motives to fix a bug
>> or uncleanliness. [...] It is absolutely fine to
>> mention checkpatch when it catches uncleanliness in code that already
>> got merged. I dont understand your point.
>
> I wrote "don't mention checkpatch" but I really meant "think about what
> the effect of the patch is and describe this".
>
> It's not really a hard problem to mention checkpatch --- it is a problem
> to make it the main point or, like in this case, the only point of the
> changelog.  (Furthermore, it is also a problem to do something routinely
> *if* doing it does not make sense.  There routinely appear coccinelle
> metapatch sources in changelogs.  That does not make sense at all, and
> doing it routinely is not a justification in itself.)
>
> So, "don't mention checkpatch" is simply a rule of thumb; read it as "I
> mentioned checkpatch in the changelog --- wait, I have possibly written
> a changelog that is besides the point; I should think about it once
> more".  :-)
>
> Now, when this particular patch is updated to get a good changelog, then
> the title could become e.g.:
>        kernel/kallsyms: change initcall level; adjust whitespace
> and anything more than that is just fluff and wasted electrons. Actually
> the changelog should rather contain a note on why device_initcall is
> supposed to be the correct initcall level.
>
> Fixes due to reports from sparse, lockdep, coverity, coccinelle, etc.
> are the in this respect the same as fixes due to reports from
> checkpatch:  Patch titles should for example be
>  - "fix potential deadlock..."
>  - "fix use-after free..."
>  - "use XYZ helper..."
>  - "adjust whitespace..."
> and *not* something like "fix lockdep backtrace" or whatever.
>
> A difference would be a patch title like "add sparse annotations"
> because this is indeed about what the patch does, not by which means it
> was created.
>
> Why do I make a fuzz?  Well, because many of our changelogs really suck
> and we need to become better in general.

Hi Stefan/Ingo/Sam and others,

Thanks a lot for all your feedbacks. It's a new learning for me and I
will ensure that I don't repeat the same mistakes next time. Will
resend all the patches with a new subject and more sensible changelog.

Thanks -
Manish


> --
> Stefan Richter
> -=====-=-=== -=-= -==-=
> http://arcgraph.de/sr/
>

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > or uncleanliness. [...] It is absolutely fine to
> > mention checkpatch when it catches uncleanliness in code that already 
> > got merged. I dont understand your point.
> 
> I wrote "don't mention checkpatch" but I really meant "think about what
> the effect of the patch is and describe this".

Are you arguing that in all those other cases the tools should not be 
mentioned either? I dont think that position is tenable.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > or uncleanliness. [...] It is absolutely fine to
> > mention checkpatch when it catches uncleanliness in code that already 
> > got merged. I dont understand your point.
> 
> I wrote "don't mention checkpatch" but I really meant "think about what
> the effect of the patch is and describe this".

Are you arguing that in all those other cases the tools should not be 
mentioned either? I dont think that position is tenable.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Manish Katiyar]

On Mon, Feb 16, 2009 at 8:52 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Ingo Molnar wrote:
>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak,
>> ftrace, kmemcheck and other tools as well when it motives to fix a bug
>> or uncleanliness. [...] It is absolutely fine to
>> mention checkpatch when it catches uncleanliness in code that already
>> got merged. I dont understand your point.
>
> I wrote "don't mention checkpatch" but I really meant "think about what
> the effect of the patch is and describe this".
>
> It's not really a hard problem to mention checkpatch --- it is a problem
> to make it the main point or, like in this case, the only point of the
> changelog.  (Furthermore, it is also a problem to do something routinely
> *if* doing it does not make sense.  There routinely appear coccinelle
> metapatch sources in changelogs.  That does not make sense at all, and
> doing it routinely is not a justification in itself.)
>
> So, "don't mention checkpatch" is simply a rule of thumb; read it as "I
> mentioned checkpatch in the changelog --- wait, I have possibly written
> a changelog that is besides the point; I should think about it once
> more".  :-)
>
> Now, when this particular patch is updated to get a good changelog, then
> the title could become e.g.:
>        kernel/kallsyms: change initcall level; adjust whitespace
> and anything more than that is just fluff and wasted electrons. Actually
> the changelog should rather contain a note on why device_initcall is
> supposed to be the correct initcall level.
>
> Fixes due to reports from sparse, lockdep, coverity, coccinelle, etc.
> are the in this respect the same as fixes due to reports from
> checkpatch:  Patch titles should for example be
>  - "fix potential deadlock..."
>  - "fix use-after free..."
>  - "use XYZ helper..."
>  - "adjust whitespace..."
> and *not* something like "fix lockdep backtrace" or whatever.
>
> A difference would be a patch title like "add sparse annotations"
> because this is indeed about what the patch does, not by which means it
> was created.
>
> Why do I make a fuzz?  Well, because many of our changelogs really suck
> and we need to become better in general.

Hi Stefan/Ingo/Sam and others,

Thanks a lot for all your feedbacks. It's a new learning for me and I
will ensure that I don't repeat the same mistakes next time. Will
resend all the patches with a new subject and more sensible changelog.

Thanks -
Manish


> --
> Stefan Richter
> -===-=-== -=-= -=-> http://arcgraph.de/sr/
>

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
>> Ingo Molnar wrote:
>>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
>>> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
>>> or uncleanliness. [...] It is absolutely fine to
>>> mention checkpatch when it catches uncleanliness in code that already 
>>> got merged. I dont understand your point.
>> I wrote "don't mention checkpatch" but I really meant "think about what
>> the effect of the patch is and describe this".
> 
> Are you arguing that in all those other cases the tools should not be 
> mentioned either? I dont think that position is tenable.

I'm arguing that in all those other cases the method "think about what
the effect of the patch is and describe this"¹ applies just as well, and
that the mentioning of the tools used does not add value for future
readers of the changelog.  When I go through changes from three or five
years ago, I need other kinds of information than patch authoring tools
that were en vogue some years ago.

Including anything relevant is the most important one of the tasks when
writing a changelog; another --- only slightly less important --- task
is to exclude anything irrelevant.

Of course what's relevant and irrelevant is in the eye of the beholder;
but the used tools + materials (scripts, static analyzers, favourite
editor, favourite crop of tea) surely are of very very low relevance.

-------------
¹) and if it not quite clear, describe also why this change is desirable
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
>> Ingo Molnar wrote:
>>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
>>> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
>>> or uncleanliness. [...] It is absolutely fine to
>>> mention checkpatch when it catches uncleanliness in code that already 
>>> got merged. I dont understand your point.
>> I wrote "don't mention checkpatch" but I really meant "think about what
>> the effect of the patch is and describe this".
> 
> Are you arguing that in all those other cases the tools should not be 
> mentioned either? I dont think that position is tenable.

I'm arguing that in all those other cases the method "think about what
the effect of the patch is and describe this"¹ applies just as well, and
that the mentioning of the tools used does not add value for future
readers of the changelog.  When I go through changes from three or five
years ago, I need other kinds of information than patch authoring tools
that were en vogue some years ago.

Including anything relevant is the most important one of the tasks when
writing a changelog; another --- only slightly less important --- task
is to exclude anything irrelevant.

Of course what's relevant and irrelevant is in the eye of the beholder;
but the used tools + materials (scripts, static analyzers, favourite
editor, favourite crop of tea) surely are of very very low relevance.

-------------
¹) and if it not quite clear, describe also why this change is desirable
-- 
Stefan Richter
-===-=-== -=-= -=-http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Al Viro]

On Mon, Feb 16, 2009 at 04:50:23PM +0100, Ingo Molnar wrote:
> 
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
> > Ingo Molnar wrote:
> > > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > > or uncleanliness. [...] It is absolutely fine to
> > > mention checkpatch when it catches uncleanliness in code that already 
> > > got merged. I dont understand your point.
> > 
> > I wrote "don't mention checkpatch" but I really meant "think about what
> > the effect of the patch is and describe this".
> 
> Are you arguing that in all those other cases the tools should not be 
> mentioned either? I dont think that position is tenable.

Hell, yes.  I'm sick and tired of "$DRIVER: fix sparse warnings <something
far off-screen when looking at it in mutt on xterm>" kind of subjects,
while we are at it.  Mention the tool when that adds information useful for
understanding commit message and patch; otherwise you are just adding noise.

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Al Viro]

On Mon, Feb 16, 2009 at 04:50:23PM +0100, Ingo Molnar wrote:
> 
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
> > Ingo Molnar wrote:
> > > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > > or uncleanliness. [...] It is absolutely fine to
> > > mention checkpatch when it catches uncleanliness in code that already 
> > > got merged. I dont understand your point.
> > 
> > I wrote "don't mention checkpatch" but I really meant "think about what
> > the effect of the patch is and describe this".
> 
> Are you arguing that in all those other cases the tools should not be 
> mentioned either? I dont think that position is tenable.

Hell, yes.  I'm sick and tired of "$DRIVER: fix sparse warnings <something
far off-screen when looking at it in mutt on xterm>" kind of subjects,
while we are at it.  Mention the tool when that adds information useful for
understanding commit message and patch; otherwise you are just adding noise.

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Julia Lawall]

On Mon, 16 Feb 2009, Ingo Molnar wrote:

> 
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
> > On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> > >> Hi Ingo,
> > >> 
> > >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> > >> Below patch removes errors generated by checkpatch.pl.
> > > When doing so use checkpatch only as a hint generator and do
> > > not concentrate only on the warnings/errors generated by checkpatch.
> > > 
> > > Your patch is an improvement but please fix the remaining issues.
> > 
> > Furthermore, the changelog is bad (non-exiting in fact).
> > 
> > The fact that the issues where discovered using checkpatch is absolutely
> > uninteresting.  The changelog should describe /what/ is fixed, e.g.
> > whitespace, maybe other things.  (In case of nontrivial changes the log
> > may also need to explain not only the /what but also the /how/, but this
> > does not apply to patches like this one.)
> 
> The commit log definitely needs enhancements but it's not uninteresting 
> at all what tools were used to arrive to a change. It shouldnt be in the 
> title, but can be mentioned in the changelog itself. (and should be 
> mentioned if the cleanup ever gets as far as the mainline kernel - if a 
> good and acceptable commit results out of a tool's usage then that tool 
> needs to be advertised some more.)

Is everything below the --- preserved in what is available via git log?  
Or at least git log -p. If so, perhaps it is indeed a good idea to move 
the "how" information there.  I think the how information has some value, 
both to make people aware of what tools are useful for what kinds of 
tasks, and to help one understand what criteria were used in making the 
patch.  This can sometimes be conveyed more precisely using code than 
English text.  I agree that the how information is not always relevant for 
the person who just wants to scan a changelog to see what is new in the 
current Linux release.

julia

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Julia Lawall]

On Mon, 16 Feb 2009, Ingo Molnar wrote:

> 
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
> > On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> > >> Hi Ingo,
> > >> 
> > >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> > >> Below patch removes errors generated by checkpatch.pl.
> > > When doing so use checkpatch only as a hint generator and do
> > > not concentrate only on the warnings/errors generated by checkpatch.
> > > 
> > > Your patch is an improvement but please fix the remaining issues.
> > 
> > Furthermore, the changelog is bad (non-exiting in fact).
> > 
> > The fact that the issues where discovered using checkpatch is absolutely
> > uninteresting.  The changelog should describe /what/ is fixed, e.g.
> > whitespace, maybe other things.  (In case of nontrivial changes the log
> > may also need to explain not only the /what but also the /how/, but this
> > does not apply to patches like this one.)
> 
> The commit log definitely needs enhancements but it's not uninteresting 
> at all what tools were used to arrive to a change. It shouldnt be in the 
> title, but can be mentioned in the changelog itself. (and should be 
> mentioned if the cleanup ever gets as far as the mainline kernel - if a 
> good and acceptable commit results out of a tool's usage then that tool 
> needs to be advertised some more.)

Is everything below the --- preserved in what is available via git log?  
Or at least git log -p. If so, perhaps it is indeed a good idea to move 
the "how" information there.  I think the how information has some value, 
both to make people aware of what tools are useful for what kinds of 
tasks, and to help one understand what criteria were used in making the 
patch.  This can sometimes be conveyed more precisely using code than 
English text.  I agree that the how information is not always relevant for 
the person who just wants to scan a changelog to see what is new in the 
current Linux release.

julia

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Julia Lawall wrote:
> Is everything below the --- preserved in what is available via git log?  

No, none of it (if the patch is mechanically applied with e.g. git-am).

Sometimes, useful information does indeed get lost because an author
didn't consider it "above ---"-worthy.

...
> I think the how information has some value, 
> both to make people aware of what tools are useful for what kinds of 
> tasks, and to help one understand what criteria were used in making the 
> patch.

That information is perfect for conservation mailing list archives.

The source repository metadata (commit logs) have other purposes:  Keep
book about what happened to the source code and why.

> This can sometimes be conveyed more precisely using code than 
> English text.

If the scope of a change is better captured that way, then OK.

> I agree that the how information is not always relevant for 
> the person who just wants to scan a changelog to see what is new in the 
> current Linux release.

The changelog is not only to learn what's new (i.e. a source of release
notes), it's also to learn what's old. :-)  I go through old changelogs
all the time because I deal with code with history older than my
involvement or too old for me to remember the circumstances.
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Julia Lawall wrote:
> Is everything below the --- preserved in what is available via git log?  

No, none of it (if the patch is mechanically applied with e.g. git-am).

Sometimes, useful information does indeed get lost because an author
didn't consider it "above ---"-worthy.

...
> I think the how information has some value, 
> both to make people aware of what tools are useful for what kinds of 
> tasks, and to help one understand what criteria were used in making the 
> patch.

That information is perfect for conservation mailing list archives.

The source repository metadata (commit logs) have other purposes:  Keep
book about what happened to the source code and why.

> This can sometimes be conveyed more precisely using code than 
> English text.

If the scope of a change is better captured that way, then OK.

> I agree that the how information is not always relevant for 
> the person who just wants to scan a changelog to see what is new in the 
> current Linux release.

The changelog is not only to learn what's new (i.e. a source of release
notes), it's also to learn what's old. :-)  I go through old changelogs
all the time because I deal with code with history older than my
involvement or too old for me to remember the circumstances.
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Mon, Feb 16, 2009 at 04:50:23PM +0100, Ingo Molnar wrote:
> > 
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> > > Ingo Molnar wrote:
> > > > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > > > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > > > or uncleanliness. [...] It is absolutely fine to
> > > > mention checkpatch when it catches uncleanliness in code that already 
> > > > got merged. I dont understand your point.
> > > 
> > > I wrote "don't mention checkpatch" but I really meant "think about what
> > > the effect of the patch is and describe this".
> > 
> > Are you arguing that in all those other cases the tools should not be 
> > mentioned either? I dont think that position is tenable.
> 
> Hell, yes.  I'm sick and tired of "$DRIVER: fix sparse warnings 
> <something far off-screen when looking at it in mutt on xterm>" kind 
> of subjects, while we are at it.  Mention the tool when that adds 
> information useful for understanding commit message and patch; 
> otherwise you are just adding noise.

No argument that it's not high value enough information to be in the 
title itself. That's why i clearly said it in my first mail:

  "It shouldnt be in the title,"

Stefan Richter's argument was different though, he argued that the 
information should not be in the changelog at all, in any place. That 
was and is my point.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Mon, Feb 16, 2009 at 04:50:23PM +0100, Ingo Molnar wrote:
> > 
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> > > Ingo Molnar wrote:
> > > > We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> > > > ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> > > > or uncleanliness. [...] It is absolutely fine to
> > > > mention checkpatch when it catches uncleanliness in code that already 
> > > > got merged. I dont understand your point.
> > > 
> > > I wrote "don't mention checkpatch" but I really meant "think about what
> > > the effect of the patch is and describe this".
> > 
> > Are you arguing that in all those other cases the tools should not be 
> > mentioned either? I dont think that position is tenable.
> 
> Hell, yes.  I'm sick and tired of "$DRIVER: fix sparse warnings 
> <something far off-screen when looking at it in mutt on xterm>" kind 
> of subjects, while we are at it.  Mention the tool when that adds 
> information useful for understanding commit message and patch; 
> otherwise you are just adding noise.

No argument that it's not high value enough information to be in the 
title itself. That's why i clearly said it in my first mail:

  "It shouldnt be in the title,"

Stefan Richter's argument was different though, he argued that the 
information should not be in the changelog at all, in any place. That 
was and is my point.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> >> Ingo Molnar wrote:
> >>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> >>> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> >>> or uncleanliness. [...] It is absolutely fine to
> >>> mention checkpatch when it catches uncleanliness in code that already 
> >>> got merged. I dont understand your point.
> >> I wrote "don't mention checkpatch" but I really meant "think about what
> >> the effect of the patch is and describe this".
> > 
> > Are you arguing that in all those other cases the tools should not be 
> > mentioned either? I dont think that position is tenable.
> 
> I'm arguing that in all those other cases the method "think about what 
> the effect of the patch is and describe this"¹ applies just as well, 
> and that the mentioning of the tools used does not add value for 
> future readers of the changelog. [...]

That position of not adding tool information to the commit log is not 
just not tenable but also incredibly silly.

Those tools are useful, they result in fixes, so why should the patch 
author pretend and hide the method of finding problems from the Git 
history? We often write "found via review" or "found via testing". It's 
useful and it gives people an idea of how certain types of fixes were 
found.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Ingo Molnar wrote:
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> >> Ingo Molnar wrote:
> >>> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> >>> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> >>> or uncleanliness. [...] It is absolutely fine to
> >>> mention checkpatch when it catches uncleanliness in code that already 
> >>> got merged. I dont understand your point.
> >> I wrote "don't mention checkpatch" but I really meant "think about what
> >> the effect of the patch is and describe this".
> > 
> > Are you arguing that in all those other cases the tools should not be 
> > mentioned either? I dont think that position is tenable.
> 
> I'm arguing that in all those other cases the method "think about what 
> the effect of the patch is and describe this"¹ applies just as well, 
> and that the mentioning of the tools used does not add value for 
> future readers of the changelog. [...]

That position of not adding tool information to the commit log is not 
just not tenable but also incredibly silly.

Those tools are useful, they result in fixes, so why should the patch 
author pretend and hide the method of finding problems from the Git 
history? We often write "found via review" or "found via testing". It's 
useful and it gives people an idea of how certain types of fixes were 
found.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Julia Lawall <julia@diku.dk> wrote:

> On Mon, 16 Feb 2009, Ingo Molnar wrote:
> 
> > 
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> > > On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > > > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> > > >> Hi Ingo,
> > > >> 
> > > >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> > > >> Below patch removes errors generated by checkpatch.pl.
> > > > When doing so use checkpatch only as a hint generator and do
> > > > not concentrate only on the warnings/errors generated by checkpatch.
> > > > 
> > > > Your patch is an improvement but please fix the remaining issues.
> > > 
> > > Furthermore, the changelog is bad (non-exiting in fact).
> > > 
> > > The fact that the issues where discovered using checkpatch is absolutely
> > > uninteresting.  The changelog should describe /what/ is fixed, e.g.
> > > whitespace, maybe other things.  (In case of nontrivial changes the log
> > > may also need to explain not only the /what but also the /how/, but this
> > > does not apply to patches like this one.)
> > 
> > The commit log definitely needs enhancements but it's not uninteresting 
> > at all what tools were used to arrive to a change. It shouldnt be in the 
> > title, but can be mentioned in the changelog itself. (and should be 
> > mentioned if the cleanup ever gets as far as the mainline kernel - if a 
> > good and acceptable commit results out of a tool's usage then that tool 
> > needs to be advertised some more.)
> 
> Is everything below the --- preserved in what is available via git log?  

No, it's lost, so the whole suggestion of putting the method of how a 
patch was motivated into the discarded section is incredibly silly. It 
should not shout in the title but is well placed somewhere in the 
changelog.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Julia Lawall <julia@diku.dk> wrote:

> On Mon, 16 Feb 2009, Ingo Molnar wrote:
> 
> > 
> > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> > 
> > > On 2/15/2009 7:47 PM, Sam Ravnborg wrote:
> > > > On Mon, Feb 16, 2009 at 12:04:36AM +0530, Manish Katiyar wrote:
> > > >> Hi Ingo,
> > > >> 
> > > >> I used your code-quality script to do cleanup in kernel/kallsyms.c.
> > > >> Below patch removes errors generated by checkpatch.pl.
> > > > When doing so use checkpatch only as a hint generator and do
> > > > not concentrate only on the warnings/errors generated by checkpatch.
> > > > 
> > > > Your patch is an improvement but please fix the remaining issues.
> > > 
> > > Furthermore, the changelog is bad (non-exiting in fact).
> > > 
> > > The fact that the issues where discovered using checkpatch is absolutely
> > > uninteresting.  The changelog should describe /what/ is fixed, e.g.
> > > whitespace, maybe other things.  (In case of nontrivial changes the log
> > > may also need to explain not only the /what but also the /how/, but this
> > > does not apply to patches like this one.)
> > 
> > The commit log definitely needs enhancements but it's not uninteresting 
> > at all what tools were used to arrive to a change. It shouldnt be in the 
> > title, but can be mentioned in the changelog itself. (and should be 
> > mentioned if the cleanup ever gets as far as the mainline kernel - if a 
> > good and acceptable commit results out of a tool's usage then that tool 
> > needs to be advertised some more.)
> 
> Is everything below the --- preserved in what is available via git log?  

No, it's lost, so the whole suggestion of putting the method of how a 
patch was motivated into the discarded section is incredibly silly. It 
should not shout in the title but is well placed somewhere in the 
changelog.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Julia Lawall wrote:
> > Is everything below the --- preserved in what is available via git log?  
> 
> No, none of it (if the patch is mechanically applied with e.g. git-am).
> 
> Sometimes, useful information does indeed get lost because an author
> didn't consider it "above ---"-worthy.
> 
> ...
> > I think the how information has some value, 
> > both to make people aware of what tools are useful for what kinds of 
> > tasks, and to help one understand what criteria were used in making the 
> > patch.
> 
> That information is perfect for conservation mailing list archives.

Uhm, a mailing list archives have numerous well-known disadvantages: 
they are not permanent or accessible in any acceptable fashion - URLs 
can go stale, certain mails get lost, the commit itself has no backlink 
to the lkml discussion [or whichever of the hundreds of maintainer lists 
it was discussed on], etc. etc.

Claiming that information is 'perfect' for the mailing list but not good 
for the Git log is a double standard.

The thing is, we need more information about how a given patch was 
originated, not less. We need more information about how efficient our 
tools are, not less.

The main problems we have with commit logs today is not too much 
information but too little information. A log message should be 
well-structured, with the more important information near the top of it, 
the less important information at the end - but arguing that something 
as fundamental as the tools that motivated a change is silly on its 
face.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in

[Ingo Molnar]

* Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Julia Lawall wrote:
> > Is everything below the --- preserved in what is available via git log?  
> 
> No, none of it (if the patch is mechanically applied with e.g. git-am).
> 
> Sometimes, useful information does indeed get lost because an author
> didn't consider it "above ---"-worthy.
> 
> ...
> > I think the how information has some value, 
> > both to make people aware of what tools are useful for what kinds of 
> > tasks, and to help one understand what criteria were used in making the 
> > patch.
> 
> That information is perfect for conservation mailing list archives.

Uhm, a mailing list archives have numerous well-known disadvantages: 
they are not permanent or accessible in any acceptable fashion - URLs 
can go stale, certain mails get lost, the commit itself has no backlink 
to the lkml discussion [or whichever of the hundreds of maintainer lists 
it was discussed on], etc. etc.

Claiming that information is 'perfect' for the mailing list but not good 
for the Git log is a double standard.

The thing is, we need more information about how a given patch was 
originated, not less. We need more information about how efficient our 
tools are, not less.

The main problems we have with commit logs today is not too much 
information but too little information. A log message should be 
well-structured, with the more important information near the top of it, 
the less important information at the end - but arguing that something 
as fundamental as the tools that motivated a change is silly on its 
face.

	Ingo

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> That position of not adding tool information to the commit log is not 
> just not tenable but also incredibly silly.

It's not a hard requirement; it's a rule of thumb to detect changelogs
which lack important information.

I made it sound like a hard requirement for simplicity's sake. :-)

(The rule works thusly:  If the author put irrelevant information into
the log, then this is a sign that insufficient care was taken and hence
there is a danger that necessary information was forgotten.)

> Those tools are useful, they result in fixes, so why should the patch 
> author pretend and hide the method of finding problems from the Git 
> history?

Because the result is what counts:  What is the change doing?  Why
change the source in this way?

The changelogs don't teach programming or discuss our development
methods.  They are not a channel for feedback from tools users to tools
developers.

I stress again that putting irrelevant information into the log is
almost as bad as forgetting relevant information.  The S/N ratio is
lowered by the latter /and/ by the former.

> We often write "found via review" or "found via testing". It's 
> useful and it gives people an idea of how certain types of fixes were 
> found.

We write it
  - to acknowledge the work which was spent by people,
  - to describe under which circumstances an issue was reproducible.

"I used script XYZ to find whitespace style deviations" is fundamentally
different from "I used this and that benchmark setup to produce the
following results" --- because there are several alternative and
*obvious* ways to find style deviations, while there may be only
non-trivial and highly specific ways to reproduce a bug or performance
result.
-- 
Stefan Richter
-=====-==--= --=- =----
http://arcgraph.de/sr/

Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

[Stefan Richter]

Ingo Molnar wrote:
> * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> That position of not adding tool information to the commit log is not 
> just not tenable but also incredibly silly.

It's not a hard requirement; it's a rule of thumb to detect changelogs
which lack important information.

I made it sound like a hard requirement for simplicity's sake. :-)

(The rule works thusly:  If the author put irrelevant information into
the log, then this is a sign that insufficient care was taken and hence
there is a danger that necessary information was forgotten.)

> Those tools are useful, they result in fixes, so why should the patch 
> author pretend and hide the method of finding problems from the Git 
> history?

Because the result is what counts:  What is the change doing?  Why
change the source in this way?

The changelogs don't teach programming or discuss our development
methods.  They are not a channel for feedback from tools users to tools
developers.

I stress again that putting irrelevant information into the log is
almost as bad as forgetting relevant information.  The S/N ratio is
lowered by the latter /and/ by the former.

> We often write "found via review" or "found via testing". It's 
> useful and it gives people an idea of how certain types of fixes were 
> found.

We write it
  - to acknowledge the work which was spent by people,
  - to describe under which circumstances an issue was reproducible.

"I used script XYZ to find whitespace style deviations" is fundamentally
different from "I used this and that benchmark setup to produce the
following results" --- because there are several alternative and
*obvious* ways to find style deviations, while there may be only
non-trivial and highly specific ways to reproduce a bug or performance
result.
-- 
Stefan Richter
-===-=--= --=- =----
http://arcgraph.de/sr/