On Wed, Aug 26, 2026 at 2:21 PM Bradley Morgan <[email protected]> wrote: > > > + /* Insert more than UID_GID_MAP_MAX_BASE_EXTENTS (e.g., 10) elements */ > > + for (i = 0; i < 10; i++) { > > + extent.first = i * 10; > > + extent.lower_first = i * 100; > > Both .first and .lower_first increase monotonically with i. So > forward[] is already sorted by .first and reverse[] is already sorted > by .lower_first before sort_idmaps() runs. The sort is a no op on > this input. > > > + /* Verify sorting is correct */ > > + for (i = 0; i < map.nr_extents; i++) { > > + KUNIT_EXPECT_EQ(test, map.forward[i].first, i * 10); > > + KUNIT_EXPECT_EQ(test, map.reverse[i].lower_first, i * 100); > > This passes even if sort_idmaps() did nothing. The test cannot tell > a working sort from a broken one. > > To actually verify sorting, insert the extents out of order so that > sort_idmaps() has to rearrange them. For example: > > extent.first = (9 - i) * 10; > extent.lower_first = (9 - i) * 100; > > Then after sort_idmaps(), forward[0].first should be 0, forward[1] > should be 10, etc. That proves the sort moved things. > > Nits: > > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, map.forward); > > Redundant. insert_extent() just allocated it on the 6th call, and > the previous KUNIT_EXPECT_EQ(test, ret, 0) already confirmed success. > If you want to keep it, at least put it after the loop where it > protects the sort, not inside the insertion loop where it tests > something that just succeeded. > > > + KUNIT_EXPECT_EQ(test, ret, 0); > > Should be KUNIT_ASSERT_EQ. If insert_extent() fails the test > continues and the next assertions run on uninitialized data. Same > for the base test. > Billionth times the charm!
I hope I've addressed everyone's concerns. Obviously let me know if I didn't. :-) > No tag yet, fix the test data and I'll tag v4. I added your "Reviewed-by" on "PATCH 1/2", but not this one. -bw > > --- Thanks! > https://lore.kernel.org/all/[email protected]/
