Add information about state #5419

[Karolina002]

This change is 

[karczex]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @karczex and @Karolina002)

---

src/test/pmreorder_stack/pmreorder_stack.c line 93 at r4 (raw file):

			fieldsp->k == 0 && fieldsp->l == 0);
	}

you should delete_markers() at the end.

---

src/test/unittest/unittest.h line 767 at r4 (raw file):

		return NULL;

	struct markers *m = malloc(sizeof(struct markers));

Please use descriptive variable names.

---

src/test/unittest/unittest.h line 771 at r4 (raw file):

	char *delim = "|";

	strncpy(s, input, strlen(input));

What's the point of this strcpy()? to count tokens you may just iterate over input.

---

src/test/unittest/unittest.h line 774 at r4 (raw file):

	m->markers_no = 0;
	while (*s)

I suggest to change this loop to something like

for(char *s = input; *s != '\0'; s++) {
		if (strcmp(s, delim) == 0)
			m->markers_no++;
	}

[karczex]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @karczex and @Karolina002)

---

src/tools/pmreorder/statemachine.py line 388 at r3 (raw file):

            self._curr_state._context.logger.info(ops)
            markers_to_pass = filter(lambda e: e[0] < ops_id[0], markers)
            markers_to_pass = map(lambda e: e[1], markers_to_pass)

Using generator expression would simplify this change little bit

markers_to_pass = (e[1] for e in markers if e[0] < ops_id[0]) 
os.environ["PMREORDER_MARKERS"] = "|".join(markers_to_pass)
``` (code not tested)

https://peps.python.org/pep-0289/

Code quote:

            markers_to_pass = filter(lambda e: e[0] < ops_id[0], markers)
            markers_to_pass = map(lambda e: e[1], markers_to_pass)
            markers_to_pass = "|".join(list(markers_to_pass))
            os.environ["PMREORDER_MARKERS"] = markers_to_pass

[Karolina002]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @karczex)

---

src/test/pmreorder_stack/pmreorder_stack.c line 93 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

you should delete_markers() at the end.

Done.

---

src/test/unittest/unittest.h line 767 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

Please use descriptive variable names.

Done.

---

src/test/unittest/unittest.h line 771 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

What's the point of this strcpy()? to count tokens you may just iterate over input.

Done.

---

src/test/unittest/unittest.h line 774 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

I suggest to change this loop to something like

for(char *s = input; *s != '\0'; s++) {
		if (strcmp(s, delim) == 0)
			m->markers_no++;
	}

Done.

---

src/tools/pmreorder/statemachine.py line 388 at r3 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

Using generator expression would simplify this change little bit

markers_to_pass = (e[1] for e in markers if e[0] < ops_id[0]) 
os.environ["PMREORDER_MARKERS"] = "|".join(markers_to_pass)
``` (code not tested)

https://peps.python.org/pep-0289/

Done.

[karczex]

src/tools/pmreorder/statemachine.py line 388 at r3 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

Done.

This is list comprehension. Did you encounter any problems with generator expression?

[Karolina002]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @karczex)

---

src/tools/pmreorder/statemachine.py line 388 at r3 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

This is list comprehension. Did you encounter any problems with generator expression?

Yes, the join() function didn't work

[igchor]

You should also extend documentation for pmreorder and mention that is sets this variable (PMEMREORDER_MARKERS) and what is the format.

Reviewed 1 of 3 files at r1, 8 of 9 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karczex and @Karolina002)

---

src/test/pmreorder_stack/pmreorder_stack.c line 61 at r11 (raw file):

	pmem_persist(&fieldsp->e, sizeof(int) * 4);

	VALGRIND_EMIT_LOG("FIELDS_PACK_TWO.END");

why this is removed?

---

src/test/pmreorder_stack/pmreorder_stack.c line 81 at r11 (raw file):

		if (log->markers_no != sm.markers_no)
			consistency = 1;
		else {

I think you can add some extra checks here (for a,b,c,d,e,f,g ...) depending on markers_no

---

src/test/unittest/unittest.h line 759 at r11 (raw file):

/*
 * get_markers - return list of the markers passed by pmreorder

can you add (e.g. in a comment) example of the input data? it would help to know what is the format provided by pmreorder

---

src/tools/pmreorder/statemachine.py line 377 at r11 (raw file):

        all_consistent = True
        for ops, ops_id in zip(operations, operation_ids):
            print(ops_id[0])

why print? if you need to print anything there are special loggers I think

[lukaszstolarczuk]

pls add (struct markers *) before malloc

[lukaszstolarczuk]

// pls apply to all

[Karolina002]

Reviewable status: 7 of 12 files reviewed, 1 unresolved discussion (waiting on @igchor, @karczex, and @lukaszstolarczuk)

---

src/test/unittest/unittest.h line 767 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

// pls apply to all

Done.

[codecov]

Codecov Report

Merging #5475 (c902f83) into master (776fde6) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c902f83 differs from pull request most recent head fc0bdca. Consider uploading reports for the commit fc0bdca to get more accurate results

@@           Coverage Diff           @@
##           master    #5475   +/-   ##
=======================================
  Coverage   72.20%   72.20%           
=======================================
  Files         193      193           
  Lines       30337    30342    +5     
=======================================
+ Hits        21904    21909    +5     
  Misses       8433     8433           

[lukaszstolarczuk]

Reviewable status: 7 of 12 files reviewed, 17 unresolved discussions (waiting on @igchor, @karczex, @Karolina002, and @lukaszstolarczuk)

---

-- commits line 2 at r14:
this commit message is rather short and not very descriptive

---

-- commits line 4 at r14:
you should rather write here Fixes: and instead of the link it's enough to write #5419

---

-- commits line 7 at r16:
this commit not only adds a new function but also extends an existing test

---

-- commits line 10 at r16:
this doc update does not make sense without the first commit - I believe these 2 should be merged

---

doc/pmreorder/pmreorder.1.md line 12 at r16 (raw file):

[comment]: <> (SPDX-License-Identifier: BSD-3-Clause)
[comment]: <> (Copyright 2018-2021, Intel Corporation)

-2022

---

doc/pmreorder/pmreorder.1.md line 46 at r16 (raw file):

Pmreorder performs the store reordering between persistent
memory barriers - a sequence of flush-fence operations.
It also sets an environmental variable -PMREORDER_MARKERS - which is

why - before variable name?

---

doc/pmreorder/pmreorder.1.md line 47 at r16 (raw file):

memory barriers - a sequence of flush-fence operations.
It also sets an environmental variable -PMREORDER_MARKERS - which is
to be further used in defining consistency.

"defining consistency"? what does it mean? 😉

---

doc/pmreorder/pmreorder.1.md line 48 at r16 (raw file):

It also sets an environmental variable -PMREORDER_MARKERS - which is
to be further used in defining consistency.
Said variable is passed as char array contains of

"variable is passed"?

---

doc/pmreorder/pmreorder.1.md line 48 at r16 (raw file):

It also sets an environmental variable -PMREORDER_MARKERS - which is
to be further used in defining consistency.
Said variable is passed as char array contains of

"contains of" -> "containing"

---

doc/pmreorder/pmreorder.1.md line 49 at r16 (raw file):

to be further used in defining consistency.
Said variable is passed as char array contains of
successive markers separated by vertical bar (‘|’).

Maybe it's just me, but based on this description I'm not sure if I would understand what the variable will contain

---

doc/pmreorder/pmreorder.1.md line 387 at r16 (raw file):

+ **PMREORDER_EMIT_LOG**=1

name of the var should probably also appear here

---

src/test/unittest/unittest.h line 671 at r16 (raw file):

};

struct markers {

perhaps this struct should be defined next to the function using it

---

src/test/unittest/unittest.h line 672 at r16 (raw file):

struct markers {
	unsigned markers_no;

size_t better?

---

src/test/unittest/unittest.h line 772 at r16 (raw file):

	struct markers *log = (struct markers *)malloc(sizeof(struct markers));
	char *delim = "|";

const char* (?)

---

src/test/unittest/unittest.h line 787 at r16 (raw file):

		strncpy(log->markers[i], token, strlen(token));
		i++;
		printf(" %s\n", token);

why this printf?

---

src/tools/pmreorder/binaryoutputhandler.py line 78 at r16 (raw file):

                self._logger.debug(
                    "Doing store in file no. {0}: {1}".format(i, bf)
                )

why these changes? If you really need to update this for some reason, pls do it in a separate commit

---

src/tools/pmreorder/statemachine.py line 377 at r11 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why print? if you need to print anything there are special loggers I think

I think you could log the markers you're going to pass to the env var.

[Karolina002]

Reviewable status: 7 of 12 files reviewed, 14 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)

---

-- commits line 2 at r14:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

this commit message is rather short and not very descriptive

Done.

---

-- commits line 4 at r14:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you should rather write here Fixes: and instead of the link it's enough to write #5419

Done.

---

-- commits line 7 at r16:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

this commit not only adds a new function but also extends an existing test

Done.

---

-- commits line 10 at r16:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

this doc update does not make sense without the first commit - I believe these 2 should be merged

Done.

---

doc/pmreorder/pmreorder.1.md line 12 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

-2022

Done.

---

doc/pmreorder/pmreorder.1.md line 46 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why - before variable name?

Done.

---

doc/pmreorder/pmreorder.1.md line 47 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

"defining consistency"? what does it mean? 😉

Done.

---

doc/pmreorder/pmreorder.1.md line 48 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

"variable is passed"?

Done.

---

doc/pmreorder/pmreorder.1.md line 48 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

"contains of" -> "containing"

Done.

---

doc/pmreorder/pmreorder.1.md line 49 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

Maybe it's just me, but based on this description I'm not sure if I would understand what the variable will contain

Done.

---

doc/pmreorder/pmreorder.1.md line 387 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

name of the var should probably also appear here

Done.

---

src/test/unittest/unittest.h line 787 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why this printf?

Done.

---

src/tools/pmreorder/binaryoutputhandler.py line 78 at r16 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why these changes? If you really need to update this for some reason, pls do it in a separate commit

Done.

---

src/tools/pmreorder/statemachine.py line 377 at r11 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think you could log the markers you're going to pass to the env var.

Done.

[lukaszstolarczuk]

Reviewable status: 2 of 12 files reviewed, 11 unresolved discussions (waiting on @igchor, @karczex, @Karolina002, and @lukaszstolarczuk)

---

-- commits line 4 at r14:

Previously, Karolina002 (Karolina Bober) wrote…

Done.

the second part is still not done

---

-- commits line 4 at r17:
I know we could use it there, but it's actually not passed "to the checker"

---

-- commits line 6 at r17:

1. it's not a proper sentence, it should be re-formatted
2. there are some misspells here - please watch out for these. There are some code spell checkers available on the market 😉
3. and what's most important - this mention of docs is quite redundant - we always update docs 😉

---

doc/pmreorder/pmreorder.1.md line 48 at r16 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

Done.

still nope - 'an env variable is a char array'? what does it mean? 😜

---

doc/pmreorder/pmreorder.1.md line 49 at r16 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

Done.

still nope, what are "successive markers"?

If I understand correctly, we only get markers visible at the current state of pmreorder - I believe at least something like that should be mentioned here

---

doc/pmreorder/pmreorder.1.md line 46 at r17 (raw file):

Pmreorder performs the store reordering between persistent
memory barriers - a sequence of flush-fence operations.
It also sets an environmental variable PMREORDER_MARKERS which is

it can be used, we don't know if it will be used

---

doc/pmreorder/pmreorder.1.md line 389 at r17 (raw file):

+ **PMREORDER_EMIT_LOG**=1

User defined markers passed from the application

an application

---

doc/pmreorder/pmreorder.1.md line 390 at r17 (raw file):

User defined markers passed from the application
are store in variable:

are stored
the variable

---

src/test/pmreorder_stack/pmreorder_stack.c line 80 at r18 (raw file):

	struct markers *log = get_markers(os_getenv("PMREORDER_MARKERS"));
	if (log) {

what about else to this if - I guess you should fatal

---

src/test/pmreorder_stack/pmreorder_stack.c line 89 at r18 (raw file):

		}
	}
	switch (log->markers_no) {

this switch and delete below should also be under if

---

src/test/unittest/unittest.h line 795 at r18 (raw file):

static inline void
delete_markers(struct markers *log)
{

+ check if log is null (?)

[pbalcer]

Reviewed 1 of 1 files at r27, all commit messages.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @Karolina002 and @lukaszstolarczuk)

---

src/test/unittest/unittest.h line 775 at r26 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

I don't think it's possible, unless user includes it in the marker name

That's what I meant - what if the users provided |MARKER.BEGIN|MARKER.END| as input?

[marcinslusarz]

Reviewable status: 11 of 12 files reviewed, 8 unresolved discussions (waiting on @Karolina002 and @lukaszstolarczuk)

---

src/test/pmreorder_stack/pmreorder_stack.c line 40 at r27 (raw file):

 * write_fields -- (internal) write data in a consistent manner.
 */
static void write_fields(struct fields *fieldsp, struct markers sm)

According to PMDK coding style, function name should go on the new line.

---

src/test/pmreorder_stack/pmreorder_stack.c line 84 at r27 (raw file):

			consistency = 1;
		else {
			for (size_t i = 0; i < (int)log->markers_no; i++)

What's the point of casting? markers_no is size_t.

---

src/test/pmreorder_stack/pmreorder_stack.c line 110 at r27 (raw file):

		delete_markers(log);
	} else {
		UT_FATAL("PMREORDER_MARKERS not found");

If you are crashing anyway, it's more concise to do it this way:

if (!log)
   UT_FATAL(...);
rest of the function

You may spare some newlines with shorter indentation...

---

src/test/unittest/unittest.h line 776 at r27 (raw file):

	log->markers_no = 1;
	for (char *s = input; *s != '\0'; s++)
		if (strncmp(s, delim, strlen(delim)) == 0)

That's a quite elaborate way of saying *s == '|' ;)

---

src/test/unittest/unittest.h line 780 at r27 (raw file):

	log->markers = MALLOC(log->markers_no * sizeof(char *));

	int i = 0;

unsigned

---

src/test/unittest/unittest.h line 784 at r27 (raw file):

	char *token;

	while ((token = strtok_r(tmp, delim, &tmp))) {

strtok_r modifies input string, so it's not possible to call this function multiple times with the same argument and get the same result, which is always surprising for a function named "get_$something".

Since this function (get_markers) belongs to the shared framework and could be called from multiple places it should be safe by default.
Please strdup it, mark input parameter as const and free it at the end.

It's probably fine in this particular case, but in principle, you should never modify environment variables.

---

src/test/unittest/unittest.h line 786 at r27 (raw file):

	while ((token = strtok_r(tmp, delim, &tmp))) {
		log->markers[i] = MALLOC(strlen(token) * sizeof(char));
		strncpy(log->markers[i], token, strlen(token));

What about the string termination character?

100% of first-time users of strncpy are wrong, so welcome to the club :) That's why I consider strncpy use as an anti-pattern.

You could fix this by including the string termination character, but I think a better fix is to use STRDUP instead.

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/pmreorder_stack/pmreorder_stack.c line 40 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

According to PMDK coding style, function name should go on the new line.

Done.

---

src/test/pmreorder_stack/pmreorder_stack.c line 84 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

What's the point of casting? markers_no is size_t.

Done.

---

src/test/pmreorder_stack/pmreorder_stack.c line 110 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

If you are crashing anyway, it's more concise to do it this way:

if (!log)
   UT_FATAL(...);
rest of the function

You may spare some newlines with shorter indentation...

Done.

---

src/test/unittest/unittest.h line 775 at r26 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

That's what I meant - what if the users provided |MARKER.BEGIN|MARKER.END| as input?

| comes from the python function, not from the user

---

src/test/unittest/unittest.h line 776 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

That's a quite elaborate way of saying *s == '|' ;)

yeah...

---

src/test/unittest/unittest.h line 780 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

unsigned

Done.

---

src/test/unittest/unittest.h line 784 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

strtok_r modifies input string, so it's not possible to call this function multiple times with the same argument and get the same result, which is always surprising for a function named "get_$something".

Since this function (get_markers) belongs to the shared framework and could be called from multiple places it should be safe by default.
Please strdup it, mark input parameter as const and free it at the end.

It's probably fine in this particular case, but in principle, you should never modify environment variables.

Done.

---

src/test/unittest/unittest.h line 786 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

What about the string termination character?

100% of first-time users of strncpy are wrong, so welcome to the club :) That's why I consider strncpy use as an anti-pattern.

You could fix this by including the string termination character, but I think a better fix is to use STRDUP instead.

Done.

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 3 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r27 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

Done.

I don't see free(tmp) anywhere?

---

src/test/unittest/unittest.h line 786 at r27 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

Done.

Well, now you allocate memory twice, and the MALLOCed value leaks...

Also, please use STRDUP instead of strdup.

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, and @pbalcer)

---

src/test/unittest/unittest.h line 773 at r28 (raw file):

	struct markers *log = (struct markers *)MALLOC(sizeof(struct markers));
	const char *delim = "|";
	char *tmp = strdup(input);

STRDUP

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

I don't see free(tmp) anywhere?

Done.

---

src/test/unittest/unittest.h line 786 at r27 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Well, now you allocate memory twice, and the MALLOCed value leaks...

Also, please use STRDUP instead of strdup.

Done.

---

src/test/unittest/unittest.h line 773 at r28 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

STRDUP

Done.

[marcinslusarz]

src/test/unittest/unittest.h line 784 at r31 (raw file):

	char *token;

	while ((token = strtok_r(tmp, delim, &tmp))) {

Heh, I should have noticed this bug earlier. strtok_r modifies the last argument, so FREE(tmp) will try to free something other than what was allocated earlier. I wonder why it doesn't blow up now...

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Heh, I should have noticed this bug earlier. strtok_r modifies the last argument, so FREE(tmp) will try to free something other than what was allocated earlier. I wonder why it doesn't blow up now...

Ah, there are no tests for PMREORDER_MARKERS in this PR, so this is dead code now.

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Ah, there are no tests for PMREORDER_MARKERS in this PR, so this is dead code now.

They are used in pmreorder_stack test.

[lukaszstolarczuk]

Reviewed all commit messages.
Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

They are used in pmreorder_stack test.

hmm.. indeed, this looks like being used in _stack test

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

hmm.. indeed, this looks like being used in _stack test

Where is PMREORDER_MARKERS set?

I looked more closely, and I see that TEST0 expects pmreorder to fail, so this test crashing on empty PMREORDER_MARKERS is counted as an expected failure. That's... not quite right. This test should also include a positive case, to prevent issues like this.

[lukaszstolarczuk]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Where is PMREORDER_MARKERS set?

I looked more closely, and I see that TEST0 expects pmreorder to fail, so this test crashing on empty PMREORDER_MARKERS is counted as an expected failure. That's... not quite right. This test should also include a positive case, to prevent issues like this.

These markers are set in pmreorder (pls see src/tools/pmreorder/statemachine.py).
Here (in the test binary) we only read them from the env variable and parse them.

as for the failure, hah, ok, looks like a good catch 😄

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

These markers are set in pmreorder (pls see src/tools/pmreorder/statemachine.py).
Here (in the test binary) we only read them from the env variable and parse them.

as for the failure, hah, ok, looks like a good catch 😄

Wow, Reviewable got even more broken since the last time I used it... It doesn't show the changes in statemachine.py at all, because "Files hidden because they have no changes or you don't need to review them". WTF.

This invalidates my claim that this code is untested, but the strtok_r issue is still valid and may lead to crashes, which will be counted as success.

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 784 at r31 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Wow, Reviewable got even more broken since the last time I used it... It doesn't show the changes in statemachine.py at all, because "Files hidden because they have no changes or you don't need to review them". WTF.

This invalidates my claim that this code is untested, but the strtok_r issue is still valid and may lead to crashes, which will be counted as success.

I fixed the problem with strtok_r. As for the empty PMREORDER_MARKERS, the test will fail.

Code snippet:

pmreorder_stack/TEST0: SETUP (check/pmem/debug/pmemcheck)
unexpected output in err0.log
err0.log below.
pmreorder_stack/TEST0 err0.log {pmreorder_stack.c:83 check_consistency} pmreorder_stack/TEST0: Error: PMREORDER_MARKERS not found

RUNTESTS: stopping: pmreorder_stack/TEST0 failed, TEST=check FS=pmem BUILD=debug
make: *** [../Makefile.inc:721: TEST0] Error 1

[pbalcer]

Reviewed 1 of 3 files at r33, all commit messages.
Reviewable status: 10 of 12 files reviewed, 4 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 775 at r26 (raw file):

Previously, Karolina002 (Karolina Bober) wrote…

| comes from the python function, not from the user

Right, but in the C layer, you don't know where the input comes from. Anyway, it's not that important...

---

src/test/unittest/unittest.h line 786 at r33 (raw file):

	for (token = strtok_r(tmp, delim, &rest); token != NULL;
		token = strtok_r(rest, delim, &rest)) {

The second call to strtok_r should have its first argument be NULL.

---

src/test/unittest/unittest.h line 788 at r33 (raw file):

		token = strtok_r(rest, delim, &rest)) {
		log->markers[i] = STRDUP(token);
		i++;

the i variable seems unused?

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 3 unresolved discussions (waiting on @Karolina002, @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 786 at r33 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

The second call to strtok_r should have its first argument be NULL.

Done.

[Karolina002]

Reviewable status: 9 of 12 files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk, @marcinslusarz, and @pbalcer)

---

src/test/unittest/unittest.h line 775 at r26 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Right, but in the C layer, you don't know where the input comes from. Anyway, it's not that important...

Done.

[marcinslusarz]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk and @pbalcer)

[pbalcer]

Reviewed 2 of 3 files at r33, 1 of 1 files at r35, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Karolina002)