obj: disable the POOL_FEAT_CHECK_BAD_BLOCKS flag

[osalyk]

https://github.com/daos-stack/pmdk/actions/runs/20616031830/job/59208884970#step:4:605

This change is 

[janekmi]

IMHO we miss this bit in order to take into account the feature bit you turn off.

diff --git a/src/common/set.c b/src/common/set.c
index f82e9fd51..24b68ae45 100644
--- a/src/common/set.c
+++ b/src/common/set.c
@@ -3042,6 +3042,9 @@ util_pool_open(struct pool_set **setp, const char *path, size_t minpartsize,
 		goto err_poolset_free;
 	}
 
+	/* filter out unsupported or turned off features */
+	compat_features &= attr->features.compat;
+
 	if (compat_features & POOL_FEAT_CHECK_BAD_BLOCKS) {
 		/* check if any bad block recovery file exists */
 		int bfe = badblocks_recovery_file_exists(set);

@janekmi reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk).

---

src/libpmemobj/obj.c line 1062 at r1 (raw file):

	} else {
		attr->features.incompat &= ~POOL_FEAT_SDS; /* off */
		attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS;

Suggestion:

attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS; /* off */

[osalyk]

@osalyk made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi).

---

src/libpmemobj/obj.c line 1062 at r1 (raw file):

	} else {
		attr->features.incompat &= ~POOL_FEAT_SDS; /* off */
		attr->features.compat &= ~POOL_FEAT_CHECK_BAD_BLOCKS;

Done.

[grom72]

@grom72 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk).

---

a discussion (no related file):
Can we have at least one test checking that the ndctl API isn't used when sds_at_create is set to 0?

[janekmi]

@janekmi made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk).

---

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Can we have at least one test checking that the ndctl API isn't used when sds_at_create is set to 1?

It seems we can. Using a static build I managed to mock the ndctl_bus_get_first() function. So, whenever it is called we can just crash a test. Names and all the rest of what a proper test should have are TBD.

The question is whether we would like to do it right now. Or at the later stage. @grom72 as we discussed previously I think it still makes sense to clean up this space after the release. Please let me remind you we have to fix the issue before the DAOS 2.8 release so we have very little time.

@osalyk what do you think? Are you up for a challenge? or would prefer to keep it reasonable? 😉

diff --git a/src/test/obj_ndctl/Makefile b/src/test/obj_ndctl/Makefile
new file mode 100644
index 000000000..232ec15b6
--- /dev/null
+++ b/src/test/obj_ndctl/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2025, Hewlett Packard Enterprise Development LP
+
+TARGET = obj_ndctl
+OBJS = obj_ndctl.o
+
+BUILD_STATIC_NONDEBUG=n
+
+LIBPMEMOBJ=y
+
+include ../Makefile.inc
+LDFLAGS += $(call extract_funcs, obj_ndctl.c)
diff --git a/src/test/obj_ndctl/obj_ndctl.c b/src/test/obj_ndctl/obj_ndctl.c
new file mode 100644
index 000000000..adfdb3cc0
--- /dev/null
+++ b/src/test/obj_ndctl/obj_ndctl.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/* Copyright 2025, Hewlett Packard Enterprise Development LP */
+
+/*
+ * obj_ndctl.c -- XXX
+ */
+
+#include <libpmemobj.h>
+#include <ndctl/libndctl.h>
+
+#include "unittest.h"
+
+FUNC_MOCK(ndctl_bus_get_first, struct ndctl_bus *, struct ndctl_ctx *ctx)
+	FUNC_MOCK_RUN_DEFAULT {
+		UT_ASSERT(0);
+	}
+FUNC_MOCK_END
+
+int
+main(int argc, char *argv[])
+{
+	START(argc, argv, "obj_ndctl");
+	if (argc < 2)
+		exit(1);
+
+	const char *path = argv[1];
+
+	PMEMobjpool *pop = pmemobj_open(path, NULL);
+	UT_ASSERTne(pop, NULL);
+
+	pmemobj_close(pop);
+
+	DONE(NULL);
+}

[janekmi]

@janekmi reviewed 22 files and all commit messages, made 7 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @osalyk).

---

src/test/pmempool_sync/TEST32 line 18 at r3 (raw file):
It is just me or something smells fishy in here? 🤣

I think here is buried the secret: 93d454d

test: test fixing bad blocks saved in recovery files on a non-pmem FS

Fixing bad blocks saved in recovery files should be tested on a non-pmem FS (because on a pmem FS it requires superuser privileges).

It was a long time ago. It may be no longer the truth. I think we should check it. If it is still not possible to run these tests as we run them in our CI right now without superuser privileges on real PMem I think these tests had to be turned off entirely. Since we cannot met both conditions at the same time require_real_pmem and require_fs_type non-pmem.

Code quote:

require_real_pmem
require_test_type medium
require_fs_type non-pmem

---

src/test/pmempool_sync/TEST32 line 40 at r3 (raw file):

expect_normal_exit "$OBJ_VERIFY$EXESUFFIX $POOLSET pmempool$SUFFIX c v &>> $LOG"

turn_on_checking_bad_blocks $POOLSET

This won't be necessary any more when we move to require_fs_type pmem, right?

---

src/test/obj_pool/TEST32 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Suggestion:

# For non-pmem POOL_FEAT_CHECK_BAD_BLOCKS is turned off via PMEMOBJ_CONF="sds.at_create=0"
require_real_pmem

---

src/test/obj_pool/TEST33 line 14 at r3 (raw file):

require_test_type medium
require_real_pmem

Suggestion:

# For non-pmem POOL_FEAT_CHECK_BAD_BLOCKS is turned off via PMEMOBJ_CONF="sds.at_create=0"
require_real_pmem

---

src/test/unittest/unittest.sh line 2622 at r3 (raw file):

		path="$DIR"
	fi
	if [ "$PMEM_IS_PMEM_FORCE" != "" ]; then

I think this is the only value we fear. In theory it could be set to 0 as well. It has its own special meaning.
Anyhow I believe the condition written like below conveys more meaning.

Suggestion:

"$PMEM_IS_PMEM_FORCE" == "1"

---

src/test/unittest/unittest.sh line 2623 at r3 (raw file):

	fi
	if [ "$PMEM_IS_PMEM_FORCE" != "" ]; then
		echo "require_real_pmem: PMEM is forced (PMEM_IS_PMEM_FORCE=$PMEM_IS_PMEM_FORCE)"

We ought to have a function doing it so all skip messages look uniformly across the code base. Just copied from require_fs_type().

Suggestion:

verbose_msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) PMEM is forced (PMEM_IS_PMEM_FORCE=$PMEM_IS_PMEM_FORCE), real PMEM required"

---

src/test/unittest/unittest.sh line 2630 at r3 (raw file):

			;;
		*)
			echo "require_real_pmem: REAL_FS=$REAL_FS, this is not real PMEM"

As above + even the message copied directly from require_fs_type() for uniformity.

Suggestion:

verbose_msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) REAL_FS $REAL_FS (pmem required)"