Use Malloc for path's buffers instead of stack

[grom72]

Decrease the amount of stack memory usage by allocating the path buffer on the heap using the Malloc() function.

Requires:

• Stack usage generation enabling. #5887

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

---

This change is 

[janekmi]

Reviewed 2 of 2 files at r1, 1 of 3 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):
I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.

---

---

src/core/out.c line 192 at r3 (raw file):

		}
		Free(log_file_pid);
	}

It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.

Code quote:

	if ((log_file = os_getenv(log_file_var)) != NULL &&
				log_file[0] != '\0') {

		/* reserve more than enough space for a PID + '\0' */
		char *log_file_pid;
		log_file_pid = Malloc(PATH_MAX);
		if (log_file_pid == NULL) {
			fprintf(stderr, "out_init !Malloc\n");
			abort();
		}
		size_t len = strlen(log_file);
		if (len > 0 && log_file[len - 1] == '-') {
			if (util_snprintf(log_file_pid, PATH_MAX, "%s%d",
					log_file, getpid()) < 0) {
				ERR("snprintf: %d", errno);
				Free(log_file_pid);
				abort();
			}
			log_file = log_file_pid;
		}

		if ((Out_fp = os_fopen(log_file, "w")) == NULL) {
			char buff[UTIL_MAX_ERR_MSG];
			util_strerror(errno, buff, UTIL_MAX_ERR_MSG);
			fprintf(stderr, "Error (%s): %s=%s: %s\n",
				log_prefix, log_file_var,
				log_file, buff);
			Free(log_file_pid);
			abort();
		}
		Free(log_file_pid);
	}

---

src/core/out.c line 199 at r3 (raw file):

#ifdef DEBUG
	static char namepath[PATH_MAX];

I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?

Code quote:

static char namepath[PATH_MAX];

---

src/core/out.c line 209 at r3 (raw file):

#ifdef DEBUG
	char *namepath;
	namepath = Malloc(PATH_MAX);

Suggestion:

	char *namepath = Malloc(PATH_MAX);

---

src/core/out.c line 215 at r3 (raw file):

	}
	LOG(1, "pid %d: program: %s", getpid(),
		util_getexecname(namepath, PATH_MAX));

Code snippet:

Free(namepath);

---

src/libpmem2/auto_flush_linux.c line 94 at r3 (raw file):

	int cpu_cache = 0;

	domain_path  = Malloc(PATH_MAX);

Suggestion:

domain_path = Malloc(PATH_MAX * sizeof(char));

---

src/libpmem2/deep_flush_linux.c line 37 at r3 (raw file):

	char rbuf[2];

	deep_flush_path  = Malloc(PATH_MAX);

Please apply consistently.

Note: Just a single space around the assignment sign (=).

Suggestion:

deep_flush_path = Malloc(PATH_MAX * sizeof(char));

---

src/libpmem2/deep_flush_linux.c line 40 at r3 (raw file):

	if (deep_flush_path == NULL) {
		ERR("!Malloc");
		ret = -1;

Suggestion:

ret = PMEM2_E_ERRNO;

---

src/libpmem2/deep_flush_linux.c line 81 at r3 (raw file):

end:
	if (deep_flush_fd > -1)

Suggestion:

deep_flush_fd != -1

---

src/libpmem2/pmem2_utils_linux.c line 47 at r3 (raw file):

	spath = Malloc(PATH_MAX);
	if (spath == NULL) {
		errno = ENOMEM;

Unnecessary. Please see other comments.

---

src/libpmem2/pmem2_utils_linux.c line 68 at r3 (raw file):

	npath = Malloc(PATH_MAX);
	if (npath == NULL) {
		errno = ENOMEM;

.

---

src/libpmem2/pmem2_utils_linux.c line 94 at r3 (raw file):

		Free(spath);
	if (npath)
		Free(npath);

Free in the reverse order they were created.

Suggestion:

	if (npath)
		Free(npath);
	if (spath)
		Free(spath);

---

src/libpmem2/region_namespace_ndctl.c line 40 at r3 (raw file):
No need to set errno by yourself.

On error, these functions return NULL and set errno.

Ref: https://man7.org/linux/man-pages/man3/malloc.3.html

---

src/libpmem2/region_namespace_ndctl.c line 43 at r3 (raw file):

		ERR("!Malloc");
		return PMEM2_E_ERRNO;

Unnecessary empty line.

---

src/libpmem2/region_namespace_ndctl.c line 65 at r3 (raw file):

	LOG(4, "found matching device: %s", path);
end:

Suggestion:

	LOG(4, "found matching device: %s", path);

end:

---

src/libpmem2/region_namespace_ndctl.c line 92 at r3 (raw file):

	path = Malloc(PATH_MAX);
	if (path == NULL) {
		errno = ENOMEM;

.

[grom72]

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

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.

Shall we keep stats files up-to-date?

---

[grom72]

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

---

src/core/out.c line 192 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.

Done.

---

src/core/out.c line 199 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?

Can we assume that out_init will be called once at a time?

---

src/core/out.c line 209 at r3 (raw file):

#ifdef DEBUG
	char *namepath;
	namepath = Malloc(PATH_MAX);

Done.
I mean use static variable here.

---

src/core/out.c line 215 at r3 (raw file):

	}
	LOG(1, "pid %d: program: %s", getpid(),
		util_getexecname(namepath, PATH_MAX));

Done.
Rollback to the original solution with static.

---

src/libpmem2/deep_flush_linux.c line 37 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please apply consistently.

Note: Just a single space around the assignment sign (=).

Done.

---

src/libpmem2/deep_flush_linux.c line 40 at r3 (raw file):

	if (deep_flush_path == NULL) {
		ERR("!Malloc");
		ret = -1;

Done.

[codecov]

Codecov Report

Merging #5888 (04a984a) into master (a08e391) will decrease coverage by 2.36%.
Report is 1 commits behind head on master.
The diff coverage is 22.53%.

❗ Current head 04a984a differs from pull request most recent head 9dfd9c0. Consider uploading reports for the commit 9dfd9c0 to get more accurate results

@@            Coverage Diff             @@
##           master    #5888      +/-   ##
==========================================
- Coverage   71.00%   68.65%   -2.36%     
==========================================
  Files         131      131              
  Lines       19172    19725     +553     
  Branches     3192     3265      +73     
==========================================
- Hits        13614    13543      -71     
- Misses       5558     6182     +624     

[grom72]

Reviewed 2 of 2 files at r1, 7 of 12 files at r4, 2 of 9 files at r5, 1 of 1 files at r6, 4 of 6 files at r7, 3 of 3 files at r8, 1 of 1 files at r9.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/libpmem2/auto_flush_linux.c line 94 at r3 (raw file):

	int cpu_cache = 0;

	domain_path  = Malloc(PATH_MAX);

Done.

---

src/libpmem2/deep_flush_linux.c line 81 at r3 (raw file):

end:
	if (deep_flush_fd > -1)

It is opposite to the previous condition ....<0

---

src/libpmem2/pmem2_utils_linux.c line 47 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary. Please see other comments.

Done.

---

src/libpmem2/pmem2_utils_linux.c line 68 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/libpmem2/pmem2_utils_linux.c line 94 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Free in the reverse order they were created.

Done.

---

src/libpmem2/region_namespace_ndctl.c line 40 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No need to set errno by yourself.

On error, these functions return NULL and set errno.

Ref: https://man7.org/linux/man-pages/man3/malloc.3.html

Done.

---

src/libpmem2/region_namespace_ndctl.c line 43 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary empty line.

Done.

---

src/libpmem2/region_namespace_ndctl.c line 65 at r3 (raw file):

	LOG(4, "found matching device: %s", path);
end:

Done.

---

src/libpmem2/region_namespace_ndctl.c line 92 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

[grom72]

Reviewed all commit messages.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

[grom72]

Reviewed 1 of 1 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: 11 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

[grom72]

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