[PATCH] afs: Fix logically dead code in afs_update_cell

[PATCH] afs: Fix logically dead code in afs_update_cell

[Gustavo A. R. Silva]

Fix logically dead code in switch statement.

Notice that *ret* is updated with -ENOMEM before the switch statement
at 395:

395                 switch (ret) {
396                 case -ENODATA:
397                 case -EDESTADDRREQ:
398                         vllist->status = DNS_LOOKUP_GOT_NOT_FOUND;
399                         break;
400                 case -EAGAIN:
401                 case -ECONNREFUSED:
402                         vllist->status = DNS_LOOKUP_GOT_TEMP_FAILURE;
403                         break;
404                 default:
405                         vllist->status = DNS_LOOKUP_GOT_LOCAL_FAILURE;
406                         break;
407                 }

hence, the code in the switch (except for the default case) makes
no sense and is logically dead.

Fix this by removing the *ret* assignment at 390:

390	ret = -ENOMEM;

which is apparently wrong.

Addresses-Coverity-ID: 1445439 ("Logically dead code")
Fixes: d5c32c89b208 ("afs: Fix cell DNS lookup")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 fs/afs/cell.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index 9c3b07ba2222..980de60bf060 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -387,7 +387,6 @@ static int afs_update_cell(struct afs_cell *cell)
 		if (ret == -ENOMEM)
 			goto out_wake;
 
-		ret = -ENOMEM;
 		vllist = afs_alloc_vlserver_list(0);
 		if (!vllist)
 			goto out_wake;
-- 
2.21.0

Re: [PATCH] afs: Fix logically dead code in afs_update_cell

[Marc Dionne]

On Mon, May 27, 2019 at 1:54 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
> Fix logically dead code in switch statement.
>
> Notice that *ret* is updated with -ENOMEM before the switch statement
> at 395:
>
> 395                 switch (ret) {
> 396                 case -ENODATA:
> 397                 case -EDESTADDRREQ:
> 398                         vllist->status = DNS_LOOKUP_GOT_NOT_FOUND;
> 399                         break;
> 400                 case -EAGAIN:
> 401                 case -ECONNREFUSED:
> 402                         vllist->status = DNS_LOOKUP_GOT_TEMP_FAILURE;
> 403                         break;
> 404                 default:
> 405                         vllist->status = DNS_LOOKUP_GOT_LOCAL_FAILURE;
> 406                         break;
> 407                 }
>
> hence, the code in the switch (except for the default case) makes
> no sense and is logically dead.
>
> Fix this by removing the *ret* assignment at 390:
>
> 390     ret = -ENOMEM;
>
> which is apparently wrong.
>
> Addresses-Coverity-ID: 1445439 ("Logically dead code")
> Fixes: d5c32c89b208 ("afs: Fix cell DNS lookup")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  fs/afs/cell.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/afs/cell.c b/fs/afs/cell.c
> index 9c3b07ba2222..980de60bf060 100644
> --- a/fs/afs/cell.c
> +++ b/fs/afs/cell.c
> @@ -387,7 +387,6 @@ static int afs_update_cell(struct afs_cell *cell)
>                 if (ret == -ENOMEM)
>                         goto out_wake;
>
> -               ret = -ENOMEM;
>                 vllist = afs_alloc_vlserver_list(0);
>                 if (!vllist)
>                         goto out_wake;

Looks like the intention here was to return -ENOMEM when
afs_alloc_vlserver_list fails, which would mean that the fix should
move the assignment within if (!vllist), rather than just removing it.
Although it might be fine to just return the error that came from
afs_dns_query instead, as you do in this patch.

Marc

Re: [PATCH] afs: Fix logically dead code in afs_update_cell

[David Howells]

Marc Dionne <marc.c.dionne@gmail.com> wrote:

> > diff --git a/fs/afs/cell.c b/fs/afs/cell.c
> > index 9c3b07ba2222..980de60bf060 100644
> > --- a/fs/afs/cell.c
> > +++ b/fs/afs/cell.c
> > @@ -387,7 +387,6 @@ static int afs_update_cell(struct afs_cell *cell)
> >                 if (ret == -ENOMEM)
> >                         goto out_wake;
> >
> > -               ret = -ENOMEM;
> >                 vllist = afs_alloc_vlserver_list(0);
> >                 if (!vllist)
> >                         goto out_wake;
> 
> Looks like the intention here was to return -ENOMEM when
> afs_alloc_vlserver_list fails, which would mean that the fix should
> move the assignment within if (!vllist), rather than just removing it.
> Although it might be fine to just return the error that came from
> afs_dns_query instead, as you do in this patch.

I think I'd rather return the original error as this patch effects.  I'm
having a ponder on it.

David