Re: [PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

On 10/14/21 3:32 AM, Tkaczyk, Mariusz wrote:

Hi Jes,

On 08.10.2021 18:05, Jes Sorensen wrote:

quoted

On 9/20/21 7:10 AM, Mateusz Grzonka wrote:

quoted

Returning NULL from get_imsm_dev or __get_imsm_dev will cause segfault.
Guarantee that it never happens.

Signed-off-by: Mateusz Grzonka <redacted>
---
  super-intel.c | 122 +++++++++++++++++++-------------------------------
  1 file changed, 46 insertions(+), 76 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 83ddc000..2c3df58a 100644
--- a/super-intel.c
+++ b/super-intel.c

@@ -858,30 +858,29 @@ static struct imsm_dev *__get_imsm_dev(struct

imsm_super *mpb, __u8 index)
      void *_mpb = mpb;
        if (index >= mpb->num_raid_devs)
-        return NULL;
+        goto error;
        /* devices start after all disks */
      offset = ((void *) &mpb->disk[mpb->num_disks]) - _mpb;
  -    for (i = 0; i <= index; i++)
+    for (i = 0; i <= index; i++, offset += sizeof_imsm_dev(_mpb +
offset, 0))
          if (i == index)
              return _mpb + offset;
-        else
-            offset += sizeof_imsm_dev(_mpb + offset, 0);
-
-    return NULL;
+error:
+    pr_err("matching failed, index: %u\n", index);
+    abort();
  }

Can we please fix the error handling properly instead of throwing in
abort() and assert() to avoid it. That's really sloppy in my book.

This situation is unexpected and shall never happen. I don't see any
advantage of handling it. Abort() is the most elegant solution.
If it happens, it shall fail loudly. Verifying that we don't get
NULL in each place where it is called is harmful and unnecessary.

Passing wrong index means that code is broken and we definitely cannot
continue.

If we are to fail with abort() at a bare minimum add some comments in
the code explaining why it's done.

Thanks,
Jes