common: skip warning: "Cannot find any matching device, no bad blocks found"

[grom72]

Skip warning: "Cannot find any matching device, no bad blocks found" when pmem2_badblock_next() is used internally.

Fixes: #6126

• Requires common: Zomzog/changelog-checker executed only for PR #6128

---

This change is 

[janekmi]

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @grom72 and @osalyk)

---

-- commits line 12 at r2:
Please rebase.

Code quote:

New commits in r2 on 10/27/2024 at 2:21 PM:
- a41b5ac: common: fix scan only for PR

  Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>

---

src/common/bad_blocks.c line 17 at r1 (raw file):

#include "vec.h"
#include "os.h"
#include "../libpmem2/badblock.h"

Ugly but true.

Suggestion:

#include "bad_blocks.h"
#include "out.h"
#include "core_assert.h"
#include "vec.h"
#include "os.h"
#include "badblocks.h"

---

src/libpmem2/badblocks_ndctl.c line 30 at r1 (raw file):

#include "set_badblocks.h"
#include "extent.h"
#include "badblock.h"

It ought to be probably plural badblocks.h.

---

src/libpmem2/badblocks_ndctl.c line 531 at r1 (raw file):

/*
 * pmem2_badblock_next -- get the next bad block

Outdated doc.

---

src/libpmem2/badblocks_ndctl.c line 534 at r1 (raw file):

 */
int
pmem2_badblock_next_int(struct pmem2_badblock_context *bbctx,

int is too integer-like to me. Can we make it unabbreviate _internal?

---

src/libpmem2/badblocks_ndctl.c line 538 at r1 (raw file):

{
	LOG(3, "bbctx %p bb %p", bbctx, bb);
	PMEM2_ERR_CLR();

Is this bit still necessary assuming there is an intermedia function when it is called by the user?

Code quote:

	LOG(3, "bbctx %p bb %p", bbctx, bb);
	PMEM2_ERR_CLR();

---

src/libpmem2/badblocks_ndctl.c line 676 at r1 (raw file):

	return 0;
}

Missing newline.

---

src/libpmem2/badblocks_ndctl.c line 683 at r1 (raw file):

	LOG(3, "bbctx %p bb %p", bbctx, bb);
	PMEM2_ERR_CLR();
	return pmem2_badblock_next_int(bbctx, bb, 1);

It goes without words that this solution is just plainly ugly. 👻

Can we make it anything like the proposed below?

So, the pmem2_badblock_next_internal() would just return error codes (no additional argument). Where some of them would also generate an error message if called from via pmem2_badblock_next()?

Suggestion:

	LOG(3, "bbctx %p bb %p", bbctx, bb);
	PMEM2_ERR_CLR();

	int ret = pmem2_badblock_next_internal(bbctx, bb);
	if (ret == PMEM2_E_NO_BAD_BLOCK_FOUND) {
		ERR_WO_ERRNO("Cannot find any matching device, no bad blocks found");
	}
	
	return ret;

---

src/libpmem2/badblocks_none.c line 47 at r1 (raw file):

}int

pmem2_badblock_next(struct pmem2_badblock_context *bbctx,

Suggestion:

}

int
pmem2_badblock_next(struct pmem2_badblock_context *bbctx,

[grom72]

Reviewed 1 of 1 files at r3, 13 of 15 files at r4.
Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @janekmi and @osalyk)

---

-- commits line 12 at r2:

Previously, janekmi (Jan Michalski) wrote…

Please rebase.

Done.

---

src/common/bad_blocks.c line 17 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Ugly but true.

common/badblocks.h -> common/bad_blocks.h
but #include "../libpmem2/badblock.h" must stay as common does not access libpmem2 by default

---

src/libpmem2/badblocks_ndctl.c line 30 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It ought to be probably plural badblocks.h.

Done.

---

src/libpmem2/badblocks_ndctl.c line 531 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Outdated doc.

Done.

---

src/libpmem2/badblocks_ndctl.c line 534 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

int is too integer-like to me. Can we make it unabbreviate _internal?

Done.

---

src/libpmem2/badblocks_ndctl.c line 538 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Is this bit still necessary assuming there is an intermedia function when it is called by the user?

Done.

---

src/libpmem2/badblocks_ndctl.c line 676 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Missing newline.

Done.

---

src/libpmem2/badblocks_ndctl.c line 683 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It goes without words that this solution is just plainly ugly. 👻

Can we make it anything like the proposed below?

So, the pmem2_badblock_next_internal() would just return error codes (no additional argument). Where some of them would also generate an error message if called from via pmem2_badblock_next()?

Done.

---

src/libpmem2/badblocks_none.c line 47 at r1 (raw file):

}int

pmem2_badblock_next(struct pmem2_badblock_context *bbctx,

Done.

[grom72]

Reviewed 1 of 4 files at r1.
Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @janekmi and @osalyk)

[janekmi]

Reviewed 1 of 1 files at r3, 15 of 15 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @grom72 and @osalyk)

---

src/common/bad_blocks.c line 17 at r4 (raw file):

#include "vec.h"
#include "os.h"
#include "../libpmem2/badblocks.h"

IMHO it would be better to add INCS += -I../libpmem2 at the end of the Makefile file to make this dependency explicit on the build system level.

Please consider.

Suggestion:

#include "badblocks.h"

---

src/libpmem2/badblocks_ndctl.c line 531 at r4 (raw file):

/*
 * pmem2_badblock_next_internal -- get the next bad block impelmentation

IMHO words referring to writing code should be banned from the source code. Obviously, it is an implementation. There are no other things you write in the source code except for implementations of various things.

Suggestion:

get the next bad block

---

src/libpmem2/badblocks_ndctl.c line 537 at r4 (raw file):

			struct pmem2_badblock *bb)
{
	LOG(4, "bbctx %p bb %p", bbctx, bb);

As far as I can tell level 3 is in use for this purpose.

Suggestion:

LOG(3, "bbctx %p bb %p", bbctx, bb);

---

src/libpmem2/badblocks_none.c line 36 at r4 (raw file):

/*
 * pmem2_badblock_next_internal -- get the next bad block implementation

.

Suggestion:

get the next bad block

---

src/common/bad_blocks.h line 77 at r4 (raw file):

#endif

#endif /* PMDK_BADBLOCKS_H */

Suggestion:

#endif /* PMDK_BAD_BLOCKS_H */

---

src/libpmem2/badblocks.h line 22 at r4 (raw file):

#endif

#endif

Suggestion:

#endif /* PMEM2_BADBLOCKS_H */

[grom72]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/common/bad_blocks.c line 17 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO it would be better to add INCS += -I../libpmem2 at the end of the Makefile file to make this dependency explicit on the build system level.

Please consider.

but such solution is already used in other common files ...

---

src/libpmem2/badblocks_ndctl.c line 537 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

As far as I can tell level 3 is in use for this purpose.

but it is not a part of public API I do not want to put such mesage on the screen

[grom72]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/libpmem2/badblocks_ndctl.c line 531 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO words referring to writing code should be banned from the source code. Obviously, it is an implementation. There are no other things you write in the source code except for implementations of various things.

Done.

---

src/libpmem2/badblocks_none.c line 36 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done

---

src/libpmem2/badblocks.h line 22 at r4 (raw file):

#endif

#endif

It is not obvious ;) but let's do it.

[grom72]

Reviewable status: 1 of 16 files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/common/bad_blocks.h line 77 at r4 (raw file):

#endif

#endif /* PMDK_BADBLOCKS_H */

Done.

[grom72]

Reviewed 1 of 15 files at r6.
Reviewable status: 2 of 16 files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

[grom72]

Reviewed 1 of 4 files at r1.
Reviewable status: 2 of 16 files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

[grom72]

Reviewed 1 of 4 files at r1, 1 of 15 files at r6.
Reviewable status: 3 of 16 files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

[grom72]

Reviewed 13 of 15 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/common/bad_blocks.c line 17 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

but such solution is already used in other common files ...

and I do not want to play with pmdk's Makefiles

[janekmi]

Reviewed 14 of 15 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

[osalyk]

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 15 of 15 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[janekmi]

Reviewed 1 of 15 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)