[Patch] Dead code in fs/9p/vfs_inode.c

[Patch] Dead code in fs/9p/vfs_inode.c

[Eric Sesterhenn]

hi,

coverity (id #971) found some dead code. In all error
cases ret is NULL, so we can remove the if statement.

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig	2006-06-29 00:50:53.000000000 +0200
+++ linux-2.6.17-git11/fs/9p/vfs_inode.c	2006-06-29 00:51:11.000000000 +0200
@@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
 
 error:
 	kfree(fcall);
-	if (ret)
-		iput(ret);
-
 	return ERR_PTR(err);
 }
 

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Russ Cox]

> coverity (id #971) found some dead code. In all error
> cases ret is NULL, so we can remove the if statement.
>
> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
>
> --- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig   2006-06-29 00:50:53.000000000 +0200
> +++ linux-2.6.17-git11/fs/9p/vfs_inode.c        2006-06-29 00:51:11.000000000 +0200
> @@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
>
>  error:
>         kfree(fcall);
> -       if (ret)
> -               iput(ret);
> -
>         return ERR_PTR(err);
>  }

What about when someone changes the code and does have ret != NULL here?
This seems like reasonable defensive programming to me.

Is the official LK policy that we can't have code that trips coverity
checks like this?

Russ

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Eric Sesterhenn / Snakebyte]

* Russ Cox (rsc@swtch.com) wrote:
> >coverity (id #971) found some dead code. In all error
> >cases ret is NULL, so we can remove the if statement.
> >
> >Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> >
> >--- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig   2006-06-29 
> >00:50:53.000000000 +0200
> >+++ linux-2.6.17-git11/fs/9p/vfs_inode.c        2006-06-29 
> >00:51:11.000000000 +0200
> >@@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
> >
> > error:
> >        kfree(fcall);
> >-       if (ret)
> >-               iput(ret);
> >-
> >        return ERR_PTR(err);
> > }
> 
> What about when someone changes the code and does have ret != NULL here?
> This seems like reasonable defensive programming to me.
> 
> Is the official LK policy that we can't have code that trips coverity
> checks like this?

If this is whats agreed upon I will no longer send patches for
such bugs, and mark them as ignore in the coverity system.
But I guess it makes also sense to remove unused code, because I
am not sure if gcc can figure out to remove it. In this case
the generated object file is 10 bytes smaller.

Eric

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Russ Cox]

> If this is whats agreed upon I will no longer send patches for
> such bugs, and mark them as ignore in the coverity system.
> But I guess it makes also sense to remove unused code, because I
> am not sure if gcc can figure out to remove it. In this case
> the generated object file is 10 bytes smaller.

I wasn't necessarily speaking for the group so much as I was interested
in how coverity was being used and what the rules were.
Thanks for the info.

Russ

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Eric Sesterhenn / Snakebyte]

* Russ Cox (rsc@swtch.com) wrote:
> >If this is whats agreed upon I will no longer send patches for
> >such bugs, and mark them as ignore in the coverity system.
> >But I guess it makes also sense to remove unused code, because I
> >am not sure if gcc can figure out to remove it. In this case
> >the generated object file is 10 bytes smaller.
> 
> I wasn't necessarily speaking for the group so much as I was interested
> in how coverity was being used and what the rules were.
> Thanks for the info.

guess thats depends on the person looking trough the bugs :)
As far as i know, there are no official rules ( except maybe
to never let gregkh see a false positive :), i just try to
take a look at the coverity reports, and fix whatever I can,
in cleanup cases like this it clearly depends on the maintainer,
what he does with the patch. the coverity guys seem to be pretty
nice in giving accounts away, so you might want to take a look
at this stuff yourself. At the moment about half of the inspected
"bugs" have been marked as invalid or false, to shut coverity up
about clearly defensive programming or false analysis, so i guess
the main part is not to make coverity shut up with a patch, since
marking it as invalid is much less hassle.

Eric

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[David Leimbach]

On 6/28/06, Eric Sesterhenn / Snakebyte <snakebyte@gmx.de> wrote:
> * Russ Cox (rsc@swtch.com) wrote:
> > >coverity (id #971) found some dead code. In all error
> > >cases ret is NULL, so we can remove the if statement.
> > >
> > >Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > >
> > >--- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig   2006-06-29
> > >00:50:53.000000000 +0200
> > >+++ linux-2.6.17-git11/fs/9p/vfs_inode.c        2006-06-29
> > >00:51:11.000000000 +0200
> > >@@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
> > >
> > > error:
> > >        kfree(fcall);
> > >-       if (ret)
> > >-               iput(ret);
> > >-
> > >        return ERR_PTR(err);
> > > }
> >
> > What about when someone changes the code and does have ret != NULL here?
> > This seems like reasonable defensive programming to me.
> >
> > Is the official LK policy that we can't have code that trips coverity
> > checks like this?
>
> If this is whats agreed upon I will no longer send patches for
> such bugs, and mark them as ignore in the coverity system.
> But I guess it makes also sense to remove unused code, because I
> am not sure if gcc can figure out to remove it. In this case
> the generated object file is 10 bytes smaller.
>
> Eric
>

I wonder if anyone cares about those 10 bytes more than the fact that
the code that generates them is written in a defensive manner. :-)

I'd be willing to give up 10 bytes to know that if things changed in
the future that check is still there :-)

Seems like a fairly meaningless optimization to me.  No offense
intended toward Eric/Snakebyte, just that sometimes things that seem
like they are optimizations and fixes end up not being either.

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Pekka Enberg]

On 6/29/06, Russ Cox <rsc@swtch.com> wrote:
> >  error:
> >         kfree(fcall);
> > -       if (ret)
> > -               iput(ret);
> > -
> >         return ERR_PTR(err);
> >  }
>
> What about when someone changes the code and does have ret != NULL here?
> This seems like reasonable defensive programming to me.

Well, you're not supposed to change code without auditing for things
like this anyway. Also, I fail to see why/how you would change
v9fs_inode_from_fid() for that to happen?  I'd say kill the check.

                                                   Pekka

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Jan Engelhardt]

>> --- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig   2006-06-29
>> 00:50:53.000000000 +0200
>> +++ linux-2.6.17-git11/fs/9p/vfs_inode.c        2006-06-29
>> 00:51:11.000000000 +0200
>> @@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
>> 
>> error:
>>        kfree(fcall);
>> -       if (ret)
>> -               iput(ret);
>> -
>>       return ERR_PTR(err);
>> }
>
> What about when someone changes the code and does have ret != NULL here?
> This seems like reasonable defensive programming to me.


How about a comment:

	kfree(fcall);

	/* Currently commented out because ret is NULL in any case.
	It is here to remind someone should this condition become
	false in future. */
	/* if(ret != NULL) */
		iput(ret);


Jan Engelhardt
-- 

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Latchesar Ionkov]

The comment is longer than the 10 bytes we save :)

I like defensive programming, but I am not sure it is required in this
case. The function is 30 lines long and it is going to be pretty hard
to make a mistake when it is modified.

I am for removing the if.

Thanks,
    Lucho

On 6/29/06, Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
> >> --- linux-2.6.17-git11/fs/9p/vfs_inode.c.orig   2006-06-29
> >> 00:50:53.000000000 +0200
> >> +++ linux-2.6.17-git11/fs/9p/vfs_inode.c        2006-06-29
> >> 00:51:11.000000000 +0200
> >> @@ -386,9 +386,6 @@ v9fs_inode_from_fid(struct v9fs_session_
> >>
> >> error:
> >>        kfree(fcall);
> >> -       if (ret)
> >> -               iput(ret);
> >> -
> >>       return ERR_PTR(err);
> >> }
> >
> > What about when someone changes the code and does have ret != NULL here?
> > This seems like reasonable defensive programming to me.
>
>
> How about a comment:
>
>         kfree(fcall);
>
>         /* Currently commented out because ret is NULL in any case.
>         It is here to remind someone should this condition become
>         false in future. */
>         /* if(ret != NULL) */
>                 iput(ret);
>
>
> Jan Engelhardt
> --
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[Jan Engelhardt]

> The comment is longer than the 10 bytes we save :)

But comments are not compiled into the final binary,
which is what I wanted to point out. So you always
save your 10 bytes in the object file.


Jan Engelhardt
-- 

Re: [V9fs-developer] [Patch] Dead code in fs/9p/vfs_inode.c

[erik quanstrom]

saving ten bytes once is not a good reason to do much of anything
in an era of multi-megabyte embedded devices.

i think the argument against code written in speculation is that
it confuses what the code does right now, may never be used and
if used, may be used in a situation that masks a real error.

- erik

Jan Engelhardt <jengelh@linux01.gwdg.de> writes

| 
| 
| > The comment is longer than the 10 bytes we save :)
| 
| But comments are not compiled into the final binary,
| which is what I wanted to point out. So you always
| save your 10 bytes in the object file.
| 
| 
| Jan Engelhardt