Core log error #5987

grom72 · 2024-01-29T10:34:16Z

Requires:

err() with explicit errno #5976

This change is

codecov · 2024-01-29T12:18:30Z

Codecov Report

Attention: 108 lines in your changes are missing coverage. Please review.

Comparison is base (4f341b7) 68.63% compared to head (a201bbd) 68.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5987      +/-   ##
==========================================
- Coverage   68.63%   68.63%   -0.01%     
==========================================
  Files         132      132              
  Lines       19587    19583       -4     
  Branches     3263     3264       +1     
==========================================
- Hits        13444    13440       -4     
  Misses       6143     6143

janekmi

Reviewed 10 of 28 files at r10, all commit messages.
Reviewable status: 10 of 111 files reviewed, 24 unresolved discussions (waiting on @grom72)

src/common/file_posix.c line 115 at r11 (raw file):

	int fd = os_open(path, O_RDONLY);
	if (fd == -1) {
		CORE_LOG_ERROR("Cannot open file %s", path);

I'm not sure what to do with this case. On this "error" the function returns 0, and the caller uses this 0 as a legit alignment. I do not know if it will fail on the API level to inform the user's app of the problem.

Let's take this one offline.

src/common/mmap.c line 94 at r11 (raw file):

	void *addr = util_map_hint(len, req_align);
	if (addr == MAP_FAILED) {
		CORE_LOG_ERROR("cannot find a contiguous region of given size");

It is just me or the output of this function is misused here:

#offline

src/common/os_deep_linux.c line 38 at r11 (raw file):

			if (ret == PMEM2_E_NOSUPP) {
				errno = ENOTSUP;
				CORE_LOG_ERROR("!deep_flush not supported");

?

Suggestion:

ERR_W_ERRNO("deep_flush not supported");

src/common/os_deep_linux.c line 155 at r11 (raw file):

				errno = ENOTSUP;
				CORE_LOG_ERROR_WITH_ERRNO(
					"deep_flush not supported");

Replace spaces with tabs.

Code quote:

				errno = ENOTSUP;
				CORE_LOG_ERROR_WITH_ERRNO(
					"deep_flush not supported");

src/common/os_deep_linux.c line 155 at r11 (raw file):

				errno = ENOTSUP;
				CORE_LOG_ERROR_WITH_ERRNO(
					"deep_flush not supported");

It seems CORE_LOG_ERROR_WITH_ERRNO() does not follow what we are currently working on in #5985 with CORE_LOG_FATAL_W_ERRNO(). Thoughts?

Code quote:

				CORE_LOG_ERROR_WITH_ERRNO(
					"deep_flush not supported");

src/common/set.c line 432 at r11 (raw file):

					part->path,
					stbuf.st_mode & ~(unsigned)S_IFMT);
			}

It generates no actual error. Warning?

Code quote:

			if (stbuf.st_mode & ~(unsigned)S_IFMT) {
				CORE_LOG_ERROR(
					"file permissions changed during pool initialization, file: %s (%o)",
					part->path,
					stbuf.st_mode & ~(unsigned)S_IFMT);
			}

src/common/set.c line 2438 at r11 (raw file):

		int bbs = badblocks_check_poolset(set, 1 /* create */);
		if (bbs < 0) {
			CORE_LOG_ERROR(

?

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 2583 at r11 (raw file):

			addr = util_map_hint(rep->resvsize, 0);
		if (addr == MAP_FAILED) {
			CORE_LOG_ERROR(

?

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 2870 at r11 (raw file):

		}
		if (bfe < 0) {
			CORE_LOG_ERROR(

?

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 2877 at r11 (raw file):

		int bbs = badblocks_check_poolset(set, 0 /* not create */);
		if (bbs < 0) {
			CORE_LOG_ERROR(

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 2885 at r11 (raw file):

			if (flags & POOL_OPEN_IGNORE_BAD_BLOCKS) {
				CORE_LOG_ERROR(
					"WARNING: pool set contains bad blocks, ignoring");

Is funny. :D But can't think of a better solution. It's ok.

Code quote:

				CORE_LOG_ERROR(
					"WARNING: pool set contains bad blocks, ignoring");

src/common/set.c line 2944 at r11 (raw file):

				CORE_LOG_ERROR(
					"!cannot open the part -- \"%s\"",
					part->path);

Tabs instead of spaces.

Code quote:

				CORE_LOG_ERROR(
					"!cannot open the part -- \"%s\"",
					part->path);

src/common/set.c line 2946 at r11 (raw file):

					part->path);
				/* try to open the next part */
				continue;

I can't make any sense of this piece.

#offline

Code quote:

				CORE_LOG_ERROR(
					"!cannot open the part -- \"%s\"",
					part->path);
				/* try to open the next part */
				continue;

src/common/set.c line 2950 at r11 (raw file):

			if (util_map_hdr(part, MAP_SHARED, 0) != 0) {
				CORE_LOG_ERROR(

?

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 3017 at r11 (raw file):

	if (util_read_compat_features(set, &compat_features)) {
		CORE_LOG_ERROR("reading compat features failed");

?

Suggestion:

ERR_WO_ERRNO

src/common/set.c line 3032 at r11 (raw file):

		if (bfe < 0) {
			CORE_LOG_ERROR(

.

src/common/set.c line 3039 at r11 (raw file):

		int bbs = badblocks_check_poolset(set, 0 /* not create */);
		if (bbs < 0) {
			CORE_LOG_ERROR(

.

src/common/set.c line 3047 at r11 (raw file):

		if (bbs > 0) {
			if (flags & POOL_OPEN_IGNORE_BAD_BLOCKS) {
				CORE_LOG_ERROR(

.

src/common/set_badblocks.c line 88 at r11 (raw file):

		CORE_LOG_ERROR("%i pool file(s) contain bad blocks",
			cfcb.n_files_bbs);
		set->has_bad_blocks = 1;

It's outrageous this variable is not consumed...

Code quote:

set->has_bad_blocks = 1;

src/common/set_badblocks.c line 213 at r11 (raw file):

				badblocks_recovery_file_alloc(set->path, r, p);
			if (rec_file == NULL) {
				CORE_LOG_ERROR(

.

src/core/util_posix.c line 51 at r11 (raw file):

			return -1;
		}
		CORE_LOG_ERROR("stat failed for %s", path1);

?

Suggestion:

ERR_WO_ERRNO

src/core/util_posix.c line 61 at r11 (raw file):

			return -1;
		}
		CORE_LOG_ERROR("stat failed for %s", path2);

.

src/libpmem/pmem_posix.c line 61 at r11 (raw file):

	if (type != MAX_PMEM_TYPE) {
		if (util_range_register(addr, len, path, type)) {
			CORE_LOG_ERROR("can't track mapped region");

?

Suggestion:

ERR_WO_ERRNO

src/libpmem2/auto_flush_linux.c line 37 at r11 (raw file):

	if ((domain_fd = os_open(domain_path, O_RDONLY)) < 0) {
		CORE_LOG_ERROR_WITH_ERRNO("open(\"%s\", O_RDONLY)",

?

Suggestion:

ERR_W_ERRNO

janekmi

Reviewable status: 10 of 111 files reviewed, 25 unresolved discussions (waiting on @grom72)

a discussion (no related file):
I'm not sure why you decided to use CORE_LOG_ERROR() instead of ERR_W(O)_ERRNO() as you have done so far. Any particular reason?

janekmi

Reviewed 18 of 28 files at r10, 1 of 1 files at r11.
Reviewable status: 23 of 111 files reviewed, 48 unresolved discussions (waiting on @grom72)

src/libpmem2/deep_flush_linux.c line 43 at r11 (raw file):

	if ((deep_flush_fd = os_open(deep_flush_path, O_RDONLY)) < 0) {
		CORE_LOG_ERROR_WITH_ERRNO("os_open(\"%s\", O_RDONLY)",
		deep_flush_path);

Indentation.

src/libpmem2/deep_flush_linux.c line 45 at r11 (raw file):

		deep_flush_path);
		return 0;
	}

Does it deserve a shiny new issue or am I missing something?

Code quote:

	if ((deep_flush_fd = os_open(deep_flush_path, O_RDONLY)) < 0) {
		CORE_LOG_ERROR_WITH_ERRNO("os_open(\"%s\", O_RDONLY)",
		deep_flush_path);
		return 0;
	}

src/libpmem2/deep_flush_linux.c line 45 at r11 (raw file):

		deep_flush_path);
		return 0;
	}

Tabs.

Code quote:

	if ((deep_flush_fd = os_open(deep_flush_path, O_RDONLY)) < 0) {
		CORE_LOG_ERROR_WITH_ERRNO("os_open(\"%s\", O_RDONLY)",
		deep_flush_path);
		return 0;
	}

src/libpmem2/deep_flush_linux.c line 50 at r11 (raw file):

		CORE_LOG_ERROR_WITH_ERRNO("read(%d)", deep_flush_fd);
		goto end;
	}

Another silent fail. Outrageous.

#issue

Code quote:

	if (read(deep_flush_fd, rbuf, sizeof(rbuf)) != 2) {
		CORE_LOG_ERROR_WITH_ERRNO("read(%d)", deep_flush_fd);
		goto end;
	}

src/libpmem2/deep_flush_linux.c line 63 at r11 (raw file):

			deep_flush_path);
		return 0;
	}

#issue

Code quote:

	if ((deep_flush_fd = os_open(deep_flush_path, O_WRONLY)) < 0) {
		CORE_LOG_ERROR("Cannot open deep_flush file %s to write",
			deep_flush_path);
		return 0;
	}

src/libpmempool/pool.c line 605 at r11 (raw file):

		ASSERT(0);
	}
#endif

Funny. All of this is just a fancy way of asserting. With no additional value whatsoever. Am I missing something?

Suggestion:

	ASSERT(dmapped >= smapped);

src/libpmempool/replica.c line 809 at r11 (raw file):

	os_fclose(recovery_file);

	CORE_LOG_ERROR("bad blocks read from the recovery file -- '%s'", path);

It is no error. I'm not sure whether the users of the library expect this message. It looks like a notice to me.

src/libpmempool/replica.c line 931 at r11 (raw file):

			}

			CORE_LOG_ERROR(

Notice?

src/libpmempool/replica.c line 945 at r11 (raw file):

			if (ret > 0) {
				CORE_LOG_ERROR(

Notice? It is no error.

src/libpmempool/sync.c line 820 at r11 (raw file):

	}

	CORE_LOG_ERROR("all bad blocks have been fixed");

Notice?

src/libpmempool/sync.c line 1327 at r11 (raw file):

		/* check if poolset is broken; if not, nothing to do */
		if (replica_is_poolset_healthy(set_hs)) {
			CORE_LOG_ERROR("poolset is healthy");

Notice?

src/libpmempool/sync.c line 1349 at r11 (raw file):

	/* in dry-run mode we can stop here */
	if (is_dry_run(flags)) {
		CORE_LOG_ERROR("Sync in dry-run mode finished successfully");

Notice?

src/libpmempool/transform.c line 58 at r11 (raw file):

	char *path = util_part_realpath(PART(rep, partn)->path);
	if (path == NULL) {
		CORE_LOG_ERROR(

Warning?

src/libpmempool/transform.c line 90 at r11 (raw file):

				errno = 0;
			}
			int result = util_compare_file_inodes(path, pathp);

#offline

Code quote:

			if (pathp == NULL) {
				if (errno != ENOENT) {
					ERR_WO_ERRNO(
						"realpath failed for %s, errno %d",
						PART(repr, p)->path, errno);
					ret = -1;
					goto out;
				}
				CORE_LOG_ERROR(
					"cannot get absolute path for %s, replica %u, part %u",
					PART(rep, partn)->path, repn, partn);
				pathp = strdup(PART(repr, p)->path);
				errno = 0;
			}
			int result = util_compare_file_inodes(path, pathp);

src/libpmempool/transform.c line 262 at r11 (raw file):

		for (unsigned ro = 0; ro < set_out->nreplicas; ++ro) {
			struct pool_replica *rep_out = REP(set_out, ro);
			CORE_LOG_ERROR("comparing rep_in %u with rep_out %u",

debug?

src/libpmempool/transform.c line 466 at r11 (raw file):

			if (exists && !rep->part[p].is_dev_dax) {
				CORE_LOG_ERROR("part file %s exists",

Notice?

src/libpmempool/transform.c line 507 at r11 (raw file):

	ssize_t pool_size = replica_get_pool_size(set_src, repn);
	if (pool_size < 0) {
		CORE_LOG_ERROR("getting pool size from replica %u failed",

Warning? Notice?

src/libpmempool/transform.c line 535 at r11 (raw file):

	ssize_t pool_size = replica_get_pool_size(set_src, repn);
	if (pool_size < 0) {
		CORE_LOG_ERROR("getting pool size from replica %u failed",

.

src/libpmempool/transform.c line 776 at r11 (raw file):

	/* create the missing headers */
	if (create_missing_headers(set_out, repn)) {
		CORE_LOG_ERROR("creating lacking headers failed: replica %u",

notice?

src/libpmempool/transform.c line 922 at r11 (raw file):

					"falling back to the input poolset failed");
			} else {
				CORE_LOG_ERROR(

notice?

src/libpmempool/transform.c line 940 at r11 (raw file):

					"falling back to the input poolset failed");
			} else {
				CORE_LOG_ERROR(

Notice?

src/test/traces/traces.c line 28 at r11 (raw file):

			MAJOR_VERSION, MINOR_VERSION);
	LOG(0, "Log level NONE");
	CORE_LOG_ERROR("Log level ERROR");

Redundant. It is just a unit test for the out.c module.

src/libpmemobj/ulog.c line 673 at r11 (raw file):

			/* this is fine, it will just use more pmem */
			CORE_LOG_ERROR(
				"unable to free transaction logs memory");

IMHO it is no error probably not even a warning. Notice? I still struggle when picking between notice and debug levels.

Code quote:

			CORE_LOG_ERROR(
				"unable to free transaction logs memory");

janekmi

Reviewed all commit messages.
Reviewable status: 23 of 111 files reviewed, 41 unresolved discussions (waiting on @grom72)

grom72

Reviewable status: 18 of 112 files reviewed, 41 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I'm not sure why you decided to use CORE_LOG_ERROR() instead of ERR_W(O)_ERRNO() as you have done so far. Any particular reason?

ERR_W(O)_ERRNO() replaces ERR()
CORE_LOG_ERROR() replaces LOG(1,...)

src/common/file_posix.c line 115 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I'm not sure what to do with this case. On this "error" the function returns 0, and the caller uses this 0 as a legit alignment. I do not know if it will fail on the API level to inform the user's app of the problem.

Let's take this one offline.

#5990

src/common/mmap.c line 94 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is just me or the output of this function is misused here:

https://github.com/pmem/pmdk/blob/master/src/test/tools/cmpmap/cmpmap.c#L203?

https://github.com/pmem/pmdk/blob/master/src/test/tools/cmpmap/cmpmap.c#L213

#offline

#5991

src/common/os_deep_linux.c line 38 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

CORE_LOG_ERROR_W_ERRNO

src/common/set.c line 432 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It generates no actual error. Warning?

Done.

src/common/set.c line 2885 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Is funny. :D But can't think of a better solution. It's ok.

Done

src/common/set.c line 2946 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I can't make any sense of this piece.

#offline

Done.

src/common/set.c line 3047 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

src/libpmem2/deep_flush_linux.c line 43 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Indentation.

Done.

src/libpmempool/pool.c line 605 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Funny. All of this is just a fancy way of asserting. With no additional value whatsoever. Am I missing something?

Should be covered by ASSERTgte macro()

src/libpmempool/replica.c line 931 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

Done.

src/libpmempool/replica.c line 945 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice? It is no error.

Done.

src/libpmempool/sync.c line 820 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

Done.

src/libpmempool/sync.c line 1327 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

Done.

src/libpmempool/sync.c line 1349 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

Done.

src/libpmempool/transform.c line 58 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Warning?

Done.

src/libpmempool/transform.c line 90 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

#offline

Done.

src/libpmempool/transform.c line 262 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

debug?

Done.

src/libpmempool/transform.c line 466 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

Error as it is interpreted as an error in

pmdk/src/libpmempool/transform.c

Line 938 in 4f341b7

if (do_added_parts_exist(set_out, set_out_hs)) {

src/libpmempool/transform.c line 507 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Warning? Notice?

Done.

src/libpmempool/transform.c line 535 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

src/libpmempool/transform.c line 776 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

notice?

No -> ret= = -1

src/libpmempool/transform.c line 922 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

notice?

Done.
It is information for the user that the recovery process has been finished.
I hope there is similar information at the beginning of this process.

src/libpmempool/transform.c line 940 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Notice?

.

src/libpmemobj/ulog.c line 673 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO it is no error probably not even a warning. Notice? I still struggle when picking between notice and debug levels.

From the user's perspective, it can be beneficial to see how many times we see this message.

grom72

Reviewable status: 18 of 112 files reviewed, 41 unresolved discussions (waiting on @janekmi)

src/common/os_deep_linux.c line 155 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems CORE_LOG_ERROR_WITH_ERRNO() does not follow what we are currently working on in #5985 with CORE_LOG_FATAL_W_ERRNO(). Thoughts?

Done.

src/common/set.c line 3017 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Skip

src/common/set.c line 3032 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Skip

src/common/set.c line 3039 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Skip

src/common/set_badblocks.c line 88 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It's outrageous this variable is not consumed...

A separate issue is needed to handle this.

src/common/set_badblocks.c line 213 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Skip

src/core/util_posix.c line 51 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Skip

src/core/util_posix.c line 61 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Skip

src/libpmem/pmem_posix.c line 61 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Skip

src/libpmem2/auto_flush_linux.c line 37 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Skip

src/libpmem2/deep_flush_linux.c line 45 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Does it deserve a shiny new issue or am I missing something?

#5992

src/libpmem2/deep_flush_linux.c line 45 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Tabs.

Done.

src/libpmem2/deep_flush_linux.c line 50 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Another silent fail. Outrageous.

#issue

#5992

src/libpmem2/deep_flush_linux.c line 63 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

#issue

#5992

src/libpmempool/replica.c line 809 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is no error. I'm not sure whether the users of the library expect this message. It looks like a notice to me.

Done.

src/test/traces/traces.c line 28 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundant. It is just a unit test for the out.c module.

But because we uses custom function it checks what it checks.

janekmi

Reviewed 1 of 6 files at r6, 83 of 87 files at r12, 2 of 5 files at r14, 1 of 1 files at r18, 4 of 5 files at r19, 2 of 3 files at r20, 1 of 1 files at r21, 4 of 4 files at r22, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @grom72)

src/core/log_internal.h line 212 at r22 (raw file):

			buff); \
	} while (0)
#endif

🐐 I don't like what has happened here. I let it go only because we already plan further clean-ups.

Code quote:

#ifdef _GNU_SOURCE
#define CORE_LOG_FATAL_W_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		CORE_LOG(CORE_LOG_LEVEL_FATAL, format ": %s", ##__VA_ARGS__, \
			strerror_r(errno, buff, CORE_LOG_MAX_ERR_MSG)); \
		abort(); \
	} while (0)
#else
#define CORE_LOG_FATAL_W_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		uint64_t ret = (uint64_t)strerror_r(errno, buff, \
			CORE_LOG_MAX_ERR_MSG); \
		ret = ret; \
		CORE_LOG(CORE_LOG_LEVEL_FATAL, format ": %s", ##__VA_ARGS__, \
			buff); \
		abort(); \
	} while (0)
#endif

#define CORE_LOG_ALWAYS(format, ...) \
	CORE_LOG(CORE_LOG_LEVEL_ALWAYS, format, ##__VA_ARGS__)

/*
 * Replacement for ERR("!*") macro (w/ errno).
 * 'f' stands here for 'function' or 'format' where the latter may accept
 * additional arguments.
 */
#ifdef _GNU_SOURCE
#define CORE_LOG_ERROR_WITH_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		CORE_LOG(CORE_LOG_LEVEL_ERROR, format ": %s", ##__VA_ARGS__, \
			strerror_r(errno, buff, CORE_LOG_MAX_ERR_MSG)); \
	} while (0)
#else
#define CORE_LOG_ERROR_WITH_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		uint64_t ret = (uint64_t)strerror_r(errno, buff, \
			CORE_LOG_MAX_ERR_MSG); \
		ret = ret; \
		CORE_LOG(CORE_LOG_LEVEL_ERROR, format ": %s", ##__VA_ARGS__, \
			buff); \
	} while (0)
#endif

#ifdef _GNU_SOURCE
#define CORE_LOG_WARNING_W_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		CORE_LOG(CORE_LOG_LEVEL_WARNING, format ": %s", ##__VA_ARGS__, \
			strerror_r(errno, buff, CORE_LOG_MAX_ERR_MSG)); \
	} while (0)
#else
#define CORE_LOG_WARNING_W_ERRNO(format, ...) \
	do { \
		char buff[CORE_LOG_MAX_ERR_MSG]; \
		uint64_t ret = (uint64_t)strerror_r(errno, buff, \
			CORE_LOG_MAX_ERR_MSG); \
		ret = ret; \
		CORE_LOG(CORE_LOG_LEVEL_WARNING, format ": %s", ##__VA_ARGS__, \
			buff); \
	} while (0)
#endif

src/libpmem2/deep_flush_linux.c line 43 at r11 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

Please do it again.

src/libpmemobj/pmalloc.c line 891 at r22 (raw file):

	enum ctl_query_source source, void *arg, struct ctl_indexes *indexes)
{
	/* suppress sunused-parameter errors */

Suggestion:

suppress unused-parameter errors

src/libpmempool/replica.c line 931 at r22 (raw file):

			}

			CORE_LOG_NOTICE(

?

Suggestion:

CORE_LOG_ALWAYS

src/libpmempool/replica.c line 1269 at r22 (raw file):

		CORE_LOG_ERROR(
			"warning: one of bad block recovery files does not exist\n"
			"         - all recovery files will be removed");

Suggestion:

		CORE_LOG_WARNING(
			"one of bad block recovery files does not exist - all recovery files will be removed");

src/libpmempool/replica.c line 1293 at r22 (raw file):

			}
		} else {
			CORE_LOG_ERROR(

It is definitely not an error.

Suggestion:

CORE_LOG_ALWAYS

src/libpmempool/replica.c line 1340 at r22 (raw file):

		if (dry_run) {
			/* dry-run - do nothing */
			CORE_LOG_ERROR("warning: bad blocks were found");

Suggestion:

CORE_LOG_WARNING("bad blocks were found");

src/libpmempool/replica.c line 1364 at r22 (raw file):

	if (dry_run) {
		/* dry-run - do nothing */
		CORE_LOG_ERROR("bad blocks would be cleared");

It is definitely not an error.

Suggestion:

CORE_LOG_ALWAYS

src/libpmempool/sync.c line 1329 at r22 (raw file):

		/* check if poolset is broken; if not, nothing to do */
		if (replica_is_poolset_healthy(set_hs)) {
			CORE_LOG_NOTICE("poolset is healthy");

I don't see any difference between these you log as notice and the one you log always.

Suggestion:

CORE_LOG_ALWAYS

src/libpmempool/sync.c line 1351 at r22 (raw file):

	/* in dry-run mode we can stop here */
	if (is_dry_run(flags)) {
		CORE_LOG_NOTICE("Sync in dry-run mode finished successfully");

.

Suggestion:

CORE_LOG_ALWAYS

src/test/util_poolset/grep1.log.match line 16 at r22 (raw file):

cannot open the part -- "$(nW)/testfile31": No such file or directory
open "$(nW)/testfile32": No such file or directory
cannot open the part -- "$(nW)/testfile32": No such file or directory

What has happened with these lines?

Code quote:

cannot open the part -- "$(nW)/testfile31": No such file or directory
open "$(nW)/testfile32": No such file or directory
cannot open the part -- "$(nW)/testfile32": No such file or directory

grom72

Reviewable status: 100 of 117 files reviewed, 10 unresolved discussions (waiting on @janekmi)

src/libpmempool/replica.c line 931 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Done.

src/libpmempool/replica.c line 1269 at r22 (raw file):

		CORE_LOG_ERROR(
			"warning: one of bad block recovery files does not exist\n"
			"         - all recovery files will be removed");

Done.

src/libpmempool/replica.c line 1293 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is definitely not an error.

Done.

src/libpmempool/replica.c line 1340 at r22 (raw file):

		if (dry_run) {
			/* dry-run - do nothing */
			CORE_LOG_ERROR("warning: bad blocks were found");

Done.

src/libpmempool/sync.c line 1329 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I don't see any difference between these you log as notice and the one you log always.

LOG_NOTICE is excluded from release in this case it is a part of normal execution no need to inform user.
in case of replica restore it is worth to inform user that some data has been restored.

src/test/util_poolset/grep1.log.match line 16 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What has happened with these lines?

They are gone.

src/libpmemobj/pmalloc.c line 891 at r22 (raw file):

	enum ctl_query_source source, void *arg, struct ctl_indexes *indexes)
{
	/* suppress sunused-parameter errors */

Done.

janekmi

Reviewed 1 of 1 files at r24, 3 of 5 files at r25, 2 of 4 files at r26, 1 of 2 files at r27, 5 of 5 files at r28, 5 of 5 files at r30, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72)

src/libpmempool/sync.c line 1329 at r22 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

LOG_NOTICE is excluded from release in this case it is a part of normal execution no need to inform user.
in case of replica restore it is worth to inform user that some data has been restored.

It's kind of a border case to run libpmempool against a healthy poolset. I would be suspicious if not get any message at all. Is it done? Or am I missing something? An error code is 0 but nothing has changed. Suspicious.

Please reconsider.

src/libpmempool/sync.c line 1351 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

The same here. I think these "control messages" were kind of an integral part of the libpmempool library and probably even more of the pmempool utility. Without them, the end user could be confused.

src/test/util_poolset/grep1.log.match line 16 at r22 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

They are gone.

I did notice. 😆 I guess I am asking why they are gone.

Ok. I have found the answer. Never mind.

Signed-off-by: Tomasz Gromadzki <[email protected]>

strerror_r() has different return type depends on compliation settings. It is included from string.h and version depends on #define _GNU_SOURCE. Signed-off-by: Tomasz Gromadzki <[email protected]>

Signed-off-by: Tomasz Gromadzki <[email protected]>

This reverts commit 8b87e93.

grom72

Reviewable status: 100 of 117 files reviewed, 1 unresolved discussion (waiting on @janekmi)

src/libpmem2/deep_flush_linux.c line 43 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please do it again.

Done.

janekmi

Reviewed 1 of 12 files at r31, 1 of 1 files at r32, 3 of 4 files at r33, 2 of 3 files at r34, 1 of 1 files at r35, 4 of 4 files at r36, 5 of 5 files at r38, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

grom72 added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Jan 29, 2024

grom72 requested a review from janekmi January 29, 2024 10:34

grom72 mentioned this pull request Jan 29, 2024

Core log warning #5988

Merged

1 task

grom72 marked this pull request as draft January 29, 2024 10:49

grom72 force-pushed the core-log-error branch from 5085531 to bf45066 Compare January 29, 2024 11:47

grom72 marked this pull request as ready for review January 29, 2024 13:39

janekmi requested changes Jan 30, 2024

View reviewed changes

grom72 force-pushed the core-log-error branch from bf45066 to ed5a677 Compare January 30, 2024 12:36

janekmi requested changes Jan 30, 2024

View reviewed changes

janekmi reviewed Jan 30, 2024

View reviewed changes

grom72 force-pushed the core-log-error branch 2 times, most recently from 4fe7959 to 50d5d47 Compare January 30, 2024 16:14

grom72 commented Jan 30, 2024

View reviewed changes

grom72 force-pushed the core-log-error branch 9 times, most recently from 58dc06e to 23a4c2c Compare January 30, 2024 22:36

janekmi requested changes Jan 31, 2024

View reviewed changes

grom72 force-pushed the core-log-error branch from 23a4c2c to 1a6d809 Compare January 31, 2024 06:55

grom72 commented Jan 31, 2024

View reviewed changes

janekmi reviewed Jan 31, 2024

View reviewed changes

grom72 added 8 commits January 31, 2024 11:51

common: convert LOG(1,...) into CORE_LOG_ERROR()

08c059b

Signed-off-by: Tomasz Gromadzki <[email protected]>

common: extend error messages in match file pmempool_transform/TEST10.

fdf8a78

Signed-off-by: Tomasz Gromadzki <[email protected]>

common: addjust loging levels

591fdf4

Signed-off-by: Tomasz Gromadzki <[email protected]>

common: CORE_LOG_WARNING_W_ERRNO implementation

ee731fa

Signed-off-by: Tomasz Gromadzki <[email protected]>

common: workaround of unpredictable strerror_r version

5e26b9f

strerror_r() has different return type depends on compliation settings. It is included from string.h and version depends on #define _GNU_SOURCE. Signed-off-by: Tomasz Gromadzki <[email protected]>

common: addjust tests match file for util_poolset tests

7d3e525

Signed-off-by: Tomasz Gromadzki <[email protected]>

common: avoid badblock error msg when used internally

2883cf1

Signed-off-by: Tomasz Gromadzki <[email protected]>

Revert "common: avoid badblock error msg when used internally"

a201bbd

This reverts commit 8b87e93.

grom72 force-pushed the core-log-error branch from 1a6d809 to a201bbd Compare January 31, 2024 11:05

grom72 commented Jan 31, 2024

View reviewed changes

janekmi approved these changes Jan 31, 2024

View reviewed changes

janekmi merged commit e67b654 into pmem:master Jan 31, 2024
7 of 8 checks passed

grom72 deleted the core-log-error branch February 15, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core log error #5987

Core log error #5987

grom72 commented Jan 29, 2024 •

edited by janekmi

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

janekmi left a comment

janekmi left a comment

janekmi left a comment

janekmi left a comment

grom72 left a comment

grom72 left a comment

janekmi left a comment

grom72 left a comment

janekmi left a comment

grom72 left a comment

janekmi left a comment

Core log error #5987

Core log error #5987

Conversation

grom72 commented Jan 29, 2024 • edited by janekmi Loading

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

grom72 commented Jan 29, 2024 •

edited by janekmi

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading