[PATCH] create_inode: fix gcc -Wall complaints

[PATCH] create_inode: fix gcc -Wall complaints

[Theodore Ts'o]

We had several functions that were not returning zero on success.  Fix
this.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 misc/create_inode.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/misc/create_inode.c b/misc/create_inode.c
index 42ac6b2..964c66a 100644
--- a/misc/create_inode.c
+++ b/misc/create_inode.c
@@ -86,10 +86,9 @@ static errcode_t set_inode_extra(ext2_filsys fs, ext2_ino_t cwd,
 	fill_inode(&inode, st);
 
 	retval = ext2fs_write_inode(fs, ino, &inode);
-	if (retval) {
+	if (retval)
 		com_err(__func__, retval, "while writing inode %u", ino);
-		return retval;
-	}
+	return retval;
 }
 
 /* Make a special file which is block, character and fifo */
@@ -115,6 +114,9 @@ errcode_t do_mknod_internal(ext2_filsys fs, ext2_ino_t cwd, const char *name,
 		mode = LINUX_S_IFIFO;
 		filetype = EXT2_FT_FIFO;
 		break;
+	default:
+		abort();
+		/* NOTREACHED */
 	}
 
 	if (!(fs->flags & EXT2_FLAG_RW)) {
@@ -204,11 +206,9 @@ try_again:
 		}
 		goto try_again;
 	}
-	if (retval) {
+	if (retval)
 		com_err("ext2fs_symlink", retval, 0);
-		return retval;
-	}

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Theodore Ts'o]

On Tue, Mar 11, 2014 at 11:41:10PM -0400, Theodore Ts'o wrote:
> We had several functions that were not returning zero on success.  Fix
> this.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

One additional gcc -Wall nit which I have not yet fixed:

create_inode.c is using ext2fs_inline_data_init() which is currently a
private function defined in ext2fsP.h.

We either need to make this a publically exported interface (in which
case we have to guarantee that it is stable), or we need to find
another way to make the right thing happen here.

Darrick, Robert, do you have any thoughts or suggestions?

	       	    	      	    	   - Ted

P.S.  My bad for not doing a gcc -Wall run on the patches before
accepting them.  And in the future, I'd appreciate it if people who
are preparing patches do a "make gcc-wall" and make sure you're not
making things worse.  There are plenty of bugs that can be turned up
this way.

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Darrick J. Wong]

On Tue, Mar 11, 2014 at 11:45:39PM -0400, Theodore Ts'o wrote:
> On Tue, Mar 11, 2014 at 11:41:10PM -0400, Theodore Ts'o wrote:
> > We had several functions that were not returning zero on success.  Fix
> > this.
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> One additional gcc -Wall nit which I have not yet fixed:
> 
> create_inode.c is using ext2fs_inline_data_init() which is currently a
> private function defined in ext2fsP.h.
> 
> We either need to make this a publically exported interface (in which
> case we have to guarantee that it is stable), or we need to find
> another way to make the right thing happen here.
> 
> Darrick, Robert, do you have any thoughts or suggestions?

I fixed that up in the cppcheck cleanups patch by moving those declarations to
ext2fs.h.  Maybe I should have shoved the patch closer to the head.

--D
> 
> 	       	    	      	    	   - Ted
> 
> P.S.  My bad for not doing a gcc -Wall run on the patches before
> accepting them.  And in the future, I'd appreciate it if people who
> are preparing patches do a "make gcc-wall" and make sure you're not
> making things worse.  There are plenty of bugs that can be turned up
> this way.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Robert Yang]

On 03/12/2014 11:48 AM, Darrick J. Wong wrote:
> On Tue, Mar 11, 2014 at 11:45:39PM -0400, Theodore Ts'o wrote:
>> On Tue, Mar 11, 2014 at 11:41:10PM -0400, Theodore Ts'o wrote:
>>> We had several functions that were not returning zero on success.  Fix
>>> this.
>>>
>>> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>>
>> One additional gcc -Wall nit which I have not yet fixed:
>>
>> create_inode.c is using ext2fs_inline_data_init() which is currently a
>> private function defined in ext2fsP.h.
>>
>> We either need to make this a publically exported interface (in which
>> case we have to guarantee that it is stable), or we need to find
>> another way to make the right thing happen here.
>>
>> Darrick, Robert, do you have any thoughts or suggestions?
>
> I fixed that up in the cppcheck cleanups patch by moving those declarations to
> ext2fs.h.  Maybe I should have shoved the patch closer to the head.
>

He Darrick and Ted,


Thank you very much!

// Robert

> --D
>>
>> 	       	    	      	    	   - Ted
>>
>> P.S.  My bad for not doing a gcc -Wall run on the patches before
>> accepting them.  And in the future, I'd appreciate it if people who
>> are preparing patches do a "make gcc-wall" and make sure you're not
>> making things worse.  There are plenty of bugs that can be turned up
>> this way.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Robert Yang]

On 03/12/2014 02:33 PM, Robert Yang wrote:
>
>
> On 03/12/2014 11:48 AM, Darrick J. Wong wrote:
>> On Tue, Mar 11, 2014 at 11:45:39PM -0400, Theodore Ts'o wrote:
>>> On Tue, Mar 11, 2014 at 11:41:10PM -0400, Theodore Ts'o wrote:
>>>> We had several functions that were not returning zero on success.  Fix
>>>> this.
>>>>
>>>> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>>>
>>> One additional gcc -Wall nit which I have not yet fixed:
>>>
>>> create_inode.c is using ext2fs_inline_data_init() which is currently a
>>> private function defined in ext2fsP.h.
>>>
>>> We either need to make this a publically exported interface (in which
>>> case we have to guarantee that it is stable), or we need to find
>>> another way to make the right thing happen here.
>>>
>>> Darrick, Robert, do you have any thoughts or suggestions?
>>
>> I fixed that up in the cppcheck cleanups patch by moving those declarations to
>> ext2fs.h.  Maybe I should have shoved the patch closer to the head.
>>
>
> He Darrick and Ted,
>

s/He/Hi/

Sorry for the typo.

// Robert

>
> Thank you very much!
>
> // Robert
>
>> --D
>>>
>>>                                             - Ted
>>>
>>> P.S.  My bad for not doing a gcc -Wall run on the patches before
>>> accepting them.  And in the future, I'd appreciate it if people who
>>> are preparing patches do a "make gcc-wall" and make sure you're not
>>> making things worse.  There are plenty of bugs that can be turned up
>>> this way.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Theodore Ts'o]

On Tue, Mar 11, 2014 at 08:48:41PM -0700, Darrick J. Wong wrote:
> 
> I fixed that up in the cppcheck cleanups patch by moving those declarations to
> ext2fs.h.  Maybe I should have shoved the patch closer to the head.

I was going to ask about cppcheck, since I'm not as familiar with it.
We have multiple static code checkers that we are available to
e2fsprogs developers:

sparse, via "make C=1"

gcc -Wall, via "make gcc-wall" and "make gcc-wall-new"

clang, via "CC=clang ./configure ; make"

converity, via Eric or Ted uploading to scan.coverity.com

... and for dynamic testing, we also have:

valgrind, via "cd build/tests ; make test_script ; ./test_script --valgrind"
	  or "... ; ./test_scripte --valgrind-leakcheck"

At this point, the problem is not that we don't have enough testing
tools --- but that we're not using them regularly.

I'm not opposed to adding cppcheck, but I'm not familiar with it ---
are there things that it catches that we might not catch via other
means?

The other thing is that if we can figure out ways to automate running
some of these tests, and perhaps detecting when there are new warnings
that have popped up, that would probably be really useful.

Also, if anyone feels moved to document ways that e2fsprogs developers
can improve their code submissions, and go hunting for bugs if they so
feel moved, that would probably be a great thing to add to the ext4
wiki.

Thanks!

						- Ted

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Darrick J. Wong]

On Wed, Mar 12, 2014 at 10:32:15AM -0400, Theodore Ts'o wrote:
> On Tue, Mar 11, 2014 at 08:48:41PM -0700, Darrick J. Wong wrote:
> > 
> > I fixed that up in the cppcheck cleanups patch by moving those declarations to
> > ext2fs.h.  Maybe I should have shoved the patch closer to the head.
> 
> I was going to ask about cppcheck, since I'm not as familiar with it.
> We have multiple static code checkers that we are available to
> e2fsprogs developers:

I've been using cppcheck (http://cppcheck.sourceforge.net/) as a poor man's
Coverity to find resource leaks, since I don't have the ability to upload code
and run checks myself.  Later I would like to use cppcheck's custom rule
feature to catch incorrect use of library functions, e.g. modifying an extent
without calling ext2fs_extent_fix_parents(), or setting i_blocks directly.

Excerpting from https://packages.debian.org/sid/cppcheck, it can catch:

 * pointers to out-of-scope auto variables;
 * assignment of auto variables to an effective parameter of a function;
 * use of deprecated functions (mktemp, gets, scanf);
 * memory leaks in class or function variables;
 * C-style pointer cast in C++ code;
 * redundant if;
 * misuse of the strtol or sprintf functions;
 * unsigned division or division by zero;
 * unused functions and struct members;
 * passing parameters by value;
 * misuse of signed char variables;
 * unusual pointer arithmetic (such as "abc" + 'd');
 * dereferenced null pointers;
 * incomplete statements;

It also seems to catch fd leaks.

> sparse, via "make C=1"
> 
> gcc -Wall, via "make gcc-wall" and "make gcc-wall-new"
> 

I didn't even realize these existed.  I rather like the idea of a make target
that builds with as much static analysis as we can muster and spits out a
report.

> clang, via "CC=clang ./configure ; make"
> 
> converity, via Eric or Ted uploading to scan.coverity.com
> 
> ... and for dynamic testing, we also have:
> 
> valgrind, via "cd build/tests ; make test_script ; ./test_script --valgrind"
> 	  or "... ; ./test_scripte --valgrind-leakcheck"
> 
> At this point, the problem is not that we don't have enough testing
> tools --- but that we're not using them regularly.

Agreed.

> I'm not opposed to adding cppcheck, but I'm not familiar with it ---
> are there things that it catches that we might not catch via other
> means?
>
> The other thing is that if we can figure out ways to automate running
> some of these tests, and perhaps detecting when there are new warnings
> that have popped up, that would probably be really useful.
> 
> Also, if anyone feels moved to document ways that e2fsprogs developers
> can improve their code submissions, and go hunting for bugs if they so
> feel moved, that would probably be a great thing to add to the ext4
> wiki.

Yes.  I'll start writing a page.
https://ext4.wiki.kernel.org/index.php?title=Ext4_Contributing

--D

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Darrick J. Wong]

On Wed, Mar 12, 2014 at 10:32:15AM -0400, Theodore Ts'o wrote:
> On Tue, Mar 11, 2014 at 08:48:41PM -0700, Darrick J. Wong wrote:
> > 
> > I fixed that up in the cppcheck cleanups patch by moving those declarations to
> > ext2fs.h.  Maybe I should have shoved the patch closer to the head.
> 
> I was going to ask about cppcheck, since I'm not as familiar with it.
> We have multiple static code checkers that we are available to
> e2fsprogs developers:
> 
> sparse, via "make C=1"
> 
> gcc -Wall, via "make gcc-wall" and "make gcc-wall-new"

Annoyingly, I ran make gcc-wall on Ubuntu 12.04 (gcc 4.6, glibc 2.15) and got
this splat:

/usr/include/x86_64-linux-gnu/sys/stat.h:456: multiple definition of `stat'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:456: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:463: multiple definition of `lstat'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:463: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:470: multiple definition of `fstat'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:470: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:478: multiple definition of `fstatat'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:478: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:486: multiple definition of `mknod'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:486: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:495: multiple definition of `mknodat'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:495: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:505: multiple definition of `stat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:505: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:512: multiple definition of `lstat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:512: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:519: multiple definition of `fstat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:519: first defined here
/usr/include/x86_64-linux-gnu/sys/stat.h:527: multiple definition of `fstatat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:527: first defined here

Not sure how to fix this, other than changing WFLAGS to start with -std=gnu99.
gcc 4.8/glibc 2.19 on Ubuntu 14 are no better.

--D
> 
> clang, via "CC=clang ./configure ; make"
> 
> converity, via Eric or Ted uploading to scan.coverity.com
> 
> ... and for dynamic testing, we also have:
> 
> valgrind, via "cd build/tests ; make test_script ; ./test_script --valgrind"
> 	  or "... ; ./test_scripte --valgrind-leakcheck"
> 
> At this point, the problem is not that we don't have enough testing
> tools --- but that we're not using them regularly.
> 
> I'm not opposed to adding cppcheck, but I'm not familiar with it ---
> are there things that it catches that we might not catch via other
> means?
> 
> The other thing is that if we can figure out ways to automate running
> some of these tests, and perhaps detecting when there are new warnings
> that have popped up, that would probably be really useful.
> 
> Also, if anyone feels moved to document ways that e2fsprogs developers
> can improve their code submissions, and go hunting for bugs if they so
> feel moved, that would probably be a great thing to add to the ext4
> wiki.
> 
> Thanks!
> 
> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Theodore Ts'o]

On Wed, Mar 12, 2014 at 01:02:19PM -0700, Darrick J. Wong wrote:
> > 
> > gcc -Wall, via "make gcc-wall" and "make gcc-wall-new"
> 
> Annoyingly, I ran make gcc-wall on Ubuntu 12.04 (gcc 4.6, glibc 2.15) and got
> this splat:
> 
> /usr/include/x86_64-linux-gnu/sys/stat.h:456: multiple definition of `stat'
> ../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:456: first defined here
> 
> Not sure how to fix this, other than changing WFLAGS to start with -std=gnu99.
> gcc 4.8/glibc 2.19 on Ubuntu 14 are no better.

Hmm, I'm using Debian testing, with gcc 4.8 and eglibc 2.18, and it's
working fine.  I'm curious, are people noticing problems with gcc-wall
on Fedora?

					- Ted

Re: [PATCH] create_inode: fix gcc -Wall complaints

[Robert Yang]

On 03/13/2014 04:51 AM, Theodore Ts'o wrote:
> On Wed, Mar 12, 2014 at 01:02:19PM -0700, Darrick J. Wong wrote:
>>>
>>> gcc -Wall, via "make gcc-wall" and "make gcc-wall-new"
>>
>> Annoyingly, I ran make gcc-wall on Ubuntu 12.04 (gcc 4.6, glibc 2.15) and got
>> this splat:
>>
>> /usr/include/x86_64-linux-gnu/sys/stat.h:456: multiple definition of `stat'
>> ../lib/libquota.a(quotaio_v2.o):/usr/include/x86_64-linux-gnu/sys/stat.h:456: first defined here
>>
>> Not sure how to fix this, other than changing WFLAGS to start with -std=gnu99.
>> gcc 4.8/glibc 2.19 on Ubuntu 14 are no better.
>
> Hmm, I'm using Debian testing, with gcc 4.8 and eglibc 2.18, and it's
> working fine.  I'm curious, are people noticing problems with gcc-wall
> on Fedora?
>

I got the similar error as Darrick on Fedora 19 x86_64:

$ make gcc-wall

[snip]
rehash.c:504:10: warning: nested extern declaration of ‘ext2fs_dirent_file_type’ 
[-Wnested-externs]
/usr/include/sys/stat.h:455: multiple definition of `stat'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:455: first defined here
/usr/include/sys/stat.h:462: multiple definition of `lstat'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:462: first defined here
/usr/include/sys/stat.h:469: multiple definition of `fstat'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:469: first defined here
/usr/include/sys/stat.h:477: multiple definition of `fstatat'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:477: first defined here
/usr/include/sys/stat.h:485: multiple definition of `mknod'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:485: first defined here
/usr/include/sys/stat.h:494: multiple definition of `mknodat'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:494: first defined here
/usr/include/sys/stat.h:504: multiple definition of `stat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:504: first defined here
/usr/include/sys/stat.h:511: multiple definition of `lstat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:511: first defined here
/usr/include/sys/stat.h:518: multiple definition of `fstat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:518: first defined here
/usr/include/sys/stat.h:526: multiple definition of `fstatat64'
../lib/libquota.a(quotaio_v2.o):/usr/include/sys/stat.h:526: first defined here
collect2: error: ld returned 1 exit status
[snip]

// Robert

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