Patchwork jffs2: fix sparse errors: directive in argument list

login
register
mail settings
Submitter Erico Nunes
Date Nov. 17, 2013, 8:18 p.m.
Message ID <1384719513-27386-1-git-send-email-nunes.erico@gmail.com>
Download mbox | patch
Permalink /patch/291878/
State New
Headers show

Comments

Erico Nunes - Nov. 17, 2013, 8:18 p.m.
This patch fixes the following errors reported when running sparse:
fs/jffs2/super.c:378:1: error: directive in argument list
fs/jffs2/super.c:380:1: error: directive in argument list
fs/jffs2/super.c:381:1: error: directive in argument list
fs/jffs2/super.c:383:1: error: directive in argument list
---
 fs/jffs2/super.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
Joe Perches - Nov. 17, 2013, 9:40 p.m.
On Sun, 2013-11-17 at 18:18 -0200, Erico Nunes wrote:
> This patch fixes the following errors reported when running sparse:
> fs/jffs2/super.c:378:1: error: directive in argument list
> fs/jffs2/super.c:380:1: error: directive in argument list
> fs/jffs2/super.c:381:1: error: directive in argument list
> fs/jffs2/super.c:383:1: error: directive in argument list

Seems like a sparse error more than anything else.
(adding sparse mailing list)

Also you need to add a "Signed-off-by:" line
with your email address.

see: Documentation/SubmittingPatches

> ---
>  fs/jffs2/super.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
> index 0defb1c..ec9e16f 100644
> --- a/fs/jffs2/super.c
> +++ b/fs/jffs2/super.c
> @@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
>  static int __init init_jffs2_fs(void)
>  {
>  	int ret;
> +	const char desc[] = "version 2.2."
> +#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
> +		" (NAND)"
> +#endif
> +#ifdef CONFIG_JFFS2_SUMMARY
> +		" (SUMMARY) "
> +#endif
> +		" © 2001-2006 Red Hat, Inc.";
>  
>  	/* Paranoia checks for on-medium structures. If we ask GCC
>  	   to pack them with __attribute__((packed)) then it _also_
> @@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
>  	BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
>  	BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
>  
> -	pr_info("version 2.2."
> -#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
> -	       " (NAND)"
> -#endif
> -#ifdef CONFIG_JFFS2_SUMMARY
> -	       " (SUMMARY) "
> -#endif
> -	       " © 2001-2006 Red Hat, Inc.\n");
> +	pr_info("%s\n", desc);
>  
>  	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
>  					     sizeof(struct jffs2_inode_info),
Erico Nunes - Nov. 17, 2013, 10:34 p.m.
Thanks for your reply.

I'll resubmit shortly.

Do you mean it as an error in the sparse tool?
I don't think so, I took a look and it seems to be designed to output
an error for that case.


On Sun, Nov 17, 2013 at 7:40 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2013-11-17 at 18:18 -0200, Erico Nunes wrote:
>> This patch fixes the following errors reported when running sparse:
>> fs/jffs2/super.c:378:1: error: directive in argument list
>> fs/jffs2/super.c:380:1: error: directive in argument list
>> fs/jffs2/super.c:381:1: error: directive in argument list
>> fs/jffs2/super.c:383:1: error: directive in argument list
>
> Seems like a sparse error more than anything else.
> (adding sparse mailing list)
>
> Also you need to add a "Signed-off-by:" line
> with your email address.
>
> see: Documentation/SubmittingPatches
>
>> ---
>>  fs/jffs2/super.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
>> index 0defb1c..ec9e16f 100644
>> --- a/fs/jffs2/super.c
>> +++ b/fs/jffs2/super.c
>> @@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
>>  static int __init init_jffs2_fs(void)
>>  {
>>       int ret;
>> +     const char desc[] = "version 2.2."
>> +#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>> +             " (NAND)"
>> +#endif
>> +#ifdef CONFIG_JFFS2_SUMMARY
>> +             " (SUMMARY) "
>> +#endif
>> +             " © 2001-2006 Red Hat, Inc.";
>>
>>       /* Paranoia checks for on-medium structures. If we ask GCC
>>          to pack them with __attribute__((packed)) then it _also_
>> @@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
>>       BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
>>       BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
>>
>> -     pr_info("version 2.2."
>> -#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>> -            " (NAND)"
>> -#endif
>> -#ifdef CONFIG_JFFS2_SUMMARY
>> -            " (SUMMARY) "
>> -#endif
>> -            " © 2001-2006 Red Hat, Inc.\n");
>> +     pr_info("%s\n", desc);
>>
>>       jffs2_inode_cachep = kmem_cache_create("jffs2_i",
>>                                            sizeof(struct jffs2_inode_info),
>
>
>
Joe Perches - Nov. 17, 2013, 10:45 p.m.
On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
> Do you mean it as an error in the sparse tool?

Yes.  I think it's a defect in how sparse
treats string concatenation.

That style:

	printk("%s\n",
#ifdef FOO
	"foo"
#endif
#ifdef BAR
	"bar"
#endif
	"string");

is pretty common in the kernel sources.

The patch itself is otherwise fine, but
perhaps unnecessary.
richard -rw- weinberger - Nov. 17, 2013, 10:53 p.m.
On Sun, Nov 17, 2013 at 11:45 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
>> Do you mean it as an error in the sparse tool?
>
> Yes.  I think it's a defect in how sparse
> treats string concatenation.
>
> That style:
>
>         printk("%s\n",
> #ifdef FOO
>         "foo"
> #endif
> #ifdef BAR
>         "bar"
> #endif
>         "string");
>
> is pretty common in the kernel sources.
>
> The patch itself is otherwise fine, but
> perhaps unnecessary.

I agree with Joe, the patch fixes a non-issue.
Al Viro - Nov. 18, 2013, 1:33 a.m.
On Sun, Nov 17, 2013 at 02:45:05PM -0800, Joe Perches wrote:
> On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
> > Do you mean it as an error in the sparse tool?
> 
> Yes.  I think it's a defect in how sparse
> treats string concatenation.
> 
> That style:
> 
> 	printk("%s\n",
> #ifdef FOO
> 	"foo"
> #endif
> #ifdef BAR
> 	"bar"
> #endif
> 	"string");
> 
> is pretty common in the kernel sources.

... and it's perfectly fine, until somebody starts playing in nasal
daemon country and do that in *macro* arguments.  And a nasal daemon
country it is - it's an undefined behaviour.  See 6.10.3p11 in C99.
And trying to define a semantics for that gets real ugly real fast.
sparse matches gcc behaviour (I hope), but it warns about such abuses.
It's a defect, all right - one being reported by sparse.

Folks, please, RTFStandard if you decide to play clever games with
preprocessing.  Chapter 6.10 is not particulary long or complicated.
C99 has improved the preprocessor semantics a whole lot compared to
the earlier horrible mess (mostly by defining it in terms of token
stream transformations rather then text ones), but it's still very
easy to abuse...

Patch

diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 0defb1c..ec9e16f 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -361,6 +361,14 @@  MODULE_ALIAS_FS("jffs2");
 static int __init init_jffs2_fs(void)
 {
 	int ret;
+	const char desc[] = "version 2.2."
+#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
+		" (NAND)"
+#endif
+#ifdef CONFIG_JFFS2_SUMMARY
+		" (SUMMARY) "
+#endif
+		" © 2001-2006 Red Hat, Inc.";
 
 	/* Paranoia checks for on-medium structures. If we ask GCC
 	   to pack them with __attribute__((packed)) then it _also_
@@ -374,14 +382,7 @@  static int __init init_jffs2_fs(void)
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
 
-	pr_info("version 2.2."
-#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
-	       " (NAND)"
-#endif
-#ifdef CONFIG_JFFS2_SUMMARY
-	       " (SUMMARY) "
-#endif
-	       " © 2001-2006 Red Hat, Inc.\n");
+	pr_info("%s\n", desc);
 
 	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
 					     sizeof(struct jffs2_inode_info),