All of lore.kernel.org
 help / color / mirror / Atom feed
* Logically dead code at fs/afs/cell.c:206
@ 2017-11-17 21:57 Gustavo A. R. Silva
  2017-11-17 22:21 ` David Howells
  0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-17 21:57 UTC (permalink / raw)
  To: David Howells; +Cc: linux-afs, linux-kernel, Gustavo A. R. Silva

Hi David,

Today Coverity reported a "Logically dead code" issue at fs/afs/cell.c:206:

        if (!excl) {
                rcu_read_lock();
                cell = afs_lookup_cell_rcu(net, name, namesz);
                rcu_read_unlock();
                if (!IS_ERR(cell)) {
                        if (excl) {
                                afs_put_cell(net, cell);
                                return ERR_PTR(-EEXIST);
                        }
                        goto wait_for_cell;
                }
        }

The problem is that when this code block is executed, the code block starting at line 211 makes no sense, as _excl_ can never be true.

I was wondering if the original intention was to null check _cell_ instead of checking _excl_. So I took a look into function afs_lookup_cell_rcu to see if _cell_ can be returned as a null pointer and at the same time the if condition at line 210 be true, but I couldn't see how that could be possible. It seems to me that when _ret_ is equal to zero, _cell_ cannot be null in afs_lookup_cell_rcu. But is case I'm wrong here and _cell_ could be null at line 210, then I think line 211 should be changed as follows:

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index 1858c91..a69a11f 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -208,7 +208,7 @@ struct afs_cell *afs_lookup_cell(struct afs_net *net,
                cell = afs_lookup_cell_rcu(net, name, namesz);
                rcu_read_unlock();
                if (!IS_ERR(cell)) {
-                       if (excl) {
+                       if (cell) {
                                afs_put_cell(net, cell);
                                return ERR_PTR(-EEXIST);
                        }

But I'm suspicious about it.

What do you think?

Thanks
--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Logically dead code at fs/afs/cell.c:206
  2017-11-17 21:57 Logically dead code at fs/afs/cell.c:206 Gustavo A. R. Silva
@ 2017-11-17 22:21 ` David Howells
  2017-11-17 22:26   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 3+ messages in thread
From: David Howells @ 2017-11-17 22:21 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: dhowells, linux-afs, linux-kernel

Gustavo A. R. Silva <garsilva@embeddedor.com> wrote:

> Today Coverity reported a "Logically dead code" issue at fs/afs/cell.c:206:
> 
>         if (!excl) {
>                 rcu_read_lock();
>                 cell = afs_lookup_cell_rcu(net, name, namesz);
>                 rcu_read_unlock();
>                 if (!IS_ERR(cell)) {
>                         if (excl) {
>                                 afs_put_cell(net, cell);
>                                 return ERR_PTR(-EEXIST);
>                         }
>                         goto wait_for_cell;
>                 }
>         }
> 
> The problem is that when this code block is executed, the code block
> starting at line 211 makes no sense, as _excl_ can never be true.

Good catch.  The interior "if (excl) { ... }" statement and body needs
removing entirely.  Originally the outer "if (!excl)" wrapping it wasn't
there.

David

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Logically dead code at fs/afs/cell.c:206
  2017-11-17 22:21 ` David Howells
@ 2017-11-17 22:26   ` Gustavo A. R. Silva
  0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-17 22:26 UTC (permalink / raw)
  To: David Howells; +Cc: linux-afs, linux-kernel


Quoting David Howells <dhowells@redhat.com>:

> Gustavo A. R. Silva <garsilva@embeddedor.com> wrote:
>
>> Today Coverity reported a "Logically dead code" issue at fs/afs/cell.c:206:
>>
>>         if (!excl) {
>>                 rcu_read_lock();
>>                 cell = afs_lookup_cell_rcu(net, name, namesz);
>>                 rcu_read_unlock();
>>                 if (!IS_ERR(cell)) {
>>                         if (excl) {
>>                                 afs_put_cell(net, cell);
>>                                 return ERR_PTR(-EEXIST);
>>                         }
>>                         goto wait_for_cell;
>>                 }
>>         }
>>
>> The problem is that when this code block is executed, the code block
>> starting at line 211 makes no sense, as _excl_ can never be true.
>
> Good catch.  The interior "if (excl) { ... }" statement and body needs
> removing entirely.  Originally the outer "if (!excl)" wrapping it wasn't
> there.
>

I'll send a patch to remove that code.

Thanks for clarifying.
--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-11-17 22:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-17 21:57 Logically dead code at fs/afs/cell.c:206 Gustavo A. R. Silva
2017-11-17 22:21 ` David Howells
2017-11-17 22:26   ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.