[PATCH] fs: dlm: Remove impossible to hit if statement

[PATCH] fs: dlm: Remove impossible to hit if statement

[Joey Pabalinas]

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
---
 fs/dlm/member.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/dlm/member.c b/fs/dlm/member.c
index be7909ead71b427aa5..bf7085f21a708ab860 100644
--- a/fs/dlm/member.c
+++ b/fs/dlm/member.c
@@ -294,19 +294,14 @@ static void add_ordered_member(struct dlm_ls
*ls, struct dlm_member *new)
  memb = list_entry(tmp, struct dlm_member, list);
  if (new->nodeid < memb->nodeid)
  break;
  }

- if (!memb)
- list_add_tail(newlist, head);
- else {
- /* FIXME: can use list macro here */
- newlist->prev = tmp->prev;
- newlist->next = tmp;
- tmp->prev->next = newlist;
- tmp->prev = newlist;
- }
+ newlist->prev = tmp->prev;
+ newlist->next = tmp;
+ tmp->prev->next = newlist;
+ tmp->prev = newlist;
 }

 static int add_remote_member(int nodeid)
 {
  int error;

base-commit: 1c41041124bd14dd6610da256a3da4e5b74ce6b1
-- 
Cheers,
Joey Pabalinas

Re: [PATCH] fs: dlm: Remove impossible to hit if statement

[Al Viro]

On Sun, Nov 05, 2023 at 12:21:49PM -1000, Joey Pabalinas wrote:
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
> ---
>  fs/dlm/member.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/dlm/member.c b/fs/dlm/member.c
> index be7909ead71b427aa5..bf7085f21a708ab860 100644
> --- a/fs/dlm/member.c
> +++ b/fs/dlm/member.c
> @@ -294,19 +294,14 @@ static void add_ordered_member(struct dlm_ls
> *ls, struct dlm_member *new)
>   memb = list_entry(tmp, struct dlm_member, list);
>   if (new->nodeid < memb->nodeid)
>   break;
>   }
> 
> - if (!memb)
> - list_add_tail(newlist, head);
> - else {
> - /* FIXME: can use list macro here */
> - newlist->prev = tmp->prev;
> - newlist->next = tmp;
> - tmp->prev->next = newlist;
> - tmp->prev = newlist;
> - }
> + newlist->prev = tmp->prev;
> + newlist->next = tmp;
> + tmp->prev->next = newlist;
> + tmp->prev = newlist;
>  }

* whitespace-damaged diff

* lack of any explanations of the reasons why patch is correct...

* ... unsurprisingly so, since it is obviously broken.

Sure, if you hit even a single iteration of that loop, you will
have memb guaranteed to be non-NULL.  Therefore, to complete the
proof you only need to consider what happens if there is not
a single iteration.  Which is to say, what happens if the list
is empty.  Well, either memb is uninitialized, or there is an
intialization somewhere upstream.  Declaration is not far before
that loop, and it is
        struct dlm_member *memb = NULL;
Er...  So for that change to be correct you need to show that
the list (ls->ls_nodes) can not be empty here.  Unfortunately,
it looks like it very much can be empty, seeing that this
is apparently the only place where elements are added to
the list in question.  So on the very first call it will
hit your "impossible to hit" case.  Which leads to...

* the patch had apparently never been tested.

NACKed-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH] fs: dlm: Remove impossible to hit if statement

[Al Viro]

On Sun, Nov 05, 2023 at 11:11:25PM +0000, Al Viro wrote:
> Sure, if you hit even a single iteration of that loop, you will
> have memb guaranteed to be non-NULL.  Therefore, to complete the
> proof you only need to consider what happens if there is not
> a single iteration.  Which is to say, what happens if the list
> is empty.  Well, either memb is uninitialized, or there is an
> intialization somewhere upstream.  Declaration is not far before
> that loop, and it is
>         struct dlm_member *memb = NULL;
> Er...  So for that change to be correct you need to show that
> the list (ls->ls_nodes) can not be empty here.  Unfortunately,
> it looks like it very much can be empty, seeing that this
> is apparently the only place where elements are added to
> the list in question.  So on the very first call it will
> hit your "impossible to hit" case.  Which leads to...
> 
> * the patch had apparently never been tested.

Looking at the uses of ->ls_nodes,  I wonder if xarray would be
a better fit here.  Might be interesting to investigate...