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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxx>