DAOS-16953 tests: fix btree parameter parsing

[grom72]

In btree.sh the -m and -t parameters were not applied correctly because of the preceding positional argument containing spaces but not wrapped in quotes

In btree.c an invalid parameter sequence was not detected due to of the partially manual parsing process.

btr_test_state introduced to provide global parameters to the test (argc/argv) and avoid static variables for that purpose.

Additionally:
instead of -t and -m parameters new parameters has been proposed:

• -R[d] for class registration, optionally with dynamic root
• -M[p] for memory registration, optionally with indication to use persistent memory

Before requesting gatekeeper:

• Two review approvals and any prior change requests have been resolved.
• Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
• Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
• Commit messages follows the guidelines outlined here.
• Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

• You are the appropriate gatekeeper to be landing the patch.
• The PR has 2 reviews by people familiar with the code, including appropriate owners.
• Githooks were used. If not, request that user install them and check copyright dates.
• Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
• All builds have passed. Check non-required builds for any new compiler warnings.
• Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
• If applicable, the PR has addressed any potential version compatibility issues.
• Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
• Extra checks if forced landing is requested
  • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
  • No new NLT or valgrind warnings. Check the classic view.
  • Quick-build or Quick-functional is not used.
• Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

---

This change is 

[janekmi]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @grom72)

---

a discussion (no related file):
Nice commit message. 👍

---

src/common/tests/btree.sh line 34 at r1 (raw file):

        dyn       Run with dynamic root
        ukey      Use integer keys
        emb       Use integer keys and embedded value

Suggestion:

Use embedded value

---

src/common/tests/btree.c line 920 at r1 (raw file):

		D_ASSERT(rc == 0);
		if (rc)
			fail_msg("Cannot setup pmem: %d\n", rc);

Either-or.

Suggestion:

		if (rc) {
			fail_msg("Cannot setup pmem: %d\n", rc);
		}

---

src/common/tests/btree.c line 926 at r1 (raw file):

		D_ASSERT(rc == 0);
		if (rc)
			fail_msg("Cannot setup vmem: %d\n", rc);

.

Suggestion:

		if (rc) {
			fail_msg("Cannot setup vmem: %d\n", rc);
		}

---

src/common/tests/btree.c line 938 at r1 (raw file):

/**
 * Execute test based on the given sequence of steps defined by
 * the test_state->argc and test_state->argv

It is nice to have a bit of documentation here. But I do not like to put all the technical details in the comment. It makes it hard to maintain.

Suggestion:

 * Execute test based on the given sequence of steps.

---

src/common/tests/btree.c line 943 at r1 (raw file):

 *    otherwise it will be ignored
 * -m parameter shall be provided before any other short parameters,
 *    otherwise it will be ignored

Kudos for pointing it out.

Code quote:

 * -t parameter shall be provided as the first parameter in the sequence,
 *    otherwise it will be ignored
 * -m parameter shall be provided before any other short parameters,
 *    otherwise it will be ignored

---

src/common/tests/btree.c line 959 at r1 (raw file):

		switch (opt) {
		case 'S':
			/* General options, not part of the test sequence */

I do not get this bit. Maybe just drop it?

Suggestion:

not part of the test sequence

---

src/common/tests/btree.c line 970 at r1 (raw file):

		case 'C':
			if (ik_utx == NULL)
				ik_btr_init_mem(false, dynamic_flag);

I would prefer to have two switches with optional arguments:

• register class
  • with an option to make it a dynamic root
• initialize memory
  • with an option to make it PMEM

Two benefits to my proposal would be:

1. You would see that registering a class and memory initialization are separate.
2. You would no have weird "leaks" between -m and -C options just to initialize the memory on time. The memory would be initialized in one place.

Please consider.

Code quote:

		case 't':
			D_PRINT("Using dynamic tree order\n");
			dynamic_flag = BTR_FEAT_DYNAMIC_ROOT;
			break;
		case 'm':
			ik_btr_init_mem(true, dynamic_flag);
			break;
		case 'C':
			if (ik_utx == NULL)
				ik_btr_init_mem(false, dynamic_flag);

---

src/common/tests/btree.c line 1069 at r1 (raw file):

	}
	if (argc != optind) {
		print_message("Invalid parameters provided.\n");

Suggestion:

print_message("Unknown parameter: %s\n", argv[optind]);

---

src/common/tests/btree.c line 1073 at r1 (raw file):

	}
	if (test_name == NULL)
		test_name = "Btree testing tool";

You might already noticed I am for having braces no matter what.

Suggestion:

	if (test_name == NULL) {
		test_name = "Btree testing tool";
	}

---

src/common/tests/btree.c line 1082 at r1 (raw file):

	daos_debug_fini();
	if (ik_utx)
		rc += utest_utx_destroy(ik_utx);

.

Suggestion:

	if (ik_utx) {
		rc += utest_utx_destroy(ik_utx);
	}

[grom72]

Reviewable status: 1 of 2 files reviewed, 9 unresolved discussions (waiting on @janekmi)

---

src/common/tests/btree.c line 920 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Either-or.

Done.

---

src/common/tests/btree.c line 926 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/common/tests/btree.c line 938 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is nice to have a bit of documentation here. But I do not like to put all the technical details in the comment. It makes it hard to maintain.

Done.

---

src/common/tests/btree.c line 959 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not get this bit. Maybe just drop it?

Done.

---

src/common/tests/btree.c line 1073 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You might already noticed I am for having braces no matter what.

Done.

---

src/common/tests/btree.c line 1082 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/common/tests/btree.sh line 34 at r1 (raw file):

        dyn       Run with dynamic root
        ukey      Use integer keys
        emb       Use integer keys and embedded value

emb also includes ukey option

---

src/common/tests/btree.c line 1069 at r1 (raw file):

	}
	if (argc != optind) {
		print_message("Invalid parameters provided.\n");

Done.

[grom72]

Reviewed 8 of 9 files at r3.
Reviewable status: 8 of 9 files reviewed, 9 unresolved discussions (waiting on @janekmi)

---

src/common/tests/btree.c line 970 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would prefer to have two switches with optional arguments:

• register class
  • with an option to make it a dynamic root
• initialize memory
  • with an option to make it PMEM

Two benefits to my proposal would be:

1. You would see that registering a class and memory initialization are separate.
2. You would no have weird "leaks" between -m and -C options just to initialize the memory on time. The memory would be initialized in one place.

Please consider.

Done.

---

src/common/tests/btree.sh line 34 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

emb also includes ukey option

Done

[grom72]

Reviewed 1 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @grom72)

---

src/common/tests/btree.c line 1073 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

Nope.

---

src/common/tests/btree.sh line 34 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done

Please do it in the first commit.

---

src/common/tests/btree.c line 892 at r3 (raw file):

    {"start-test", required_argument, NULL, 'S'},
    {"reg_class", optional_argument, NULL, 'R'},
    {"reg_memory", optional_argument, NULL, 'M'},

Suggestion:

    {"reg-class", optional_argument, NULL, 'R'},
    {"reg-memory", optional_argument, NULL, 'M'},

---

src/common/tests/btree.c line 916 at r3 (raw file):

	int rc = 0;
	if (pmem_flag && *pmem_flag != 'p') {
		fail_msg("Invalid value of -M parameter: %c\n", *pmem_flag);

IMHO it is a string not a character.

Suggestion:

fail_msg("Invalid value of -M parameter: %s\n", pmem_flag);

---

src/common/tests/btree.c line 940 at r3 (raw file):

{
	if (dynamic_flag && *dynamic_flag != 'd') {
		fail_msg("Invalid value of -R parameter: %c\n", *dynamic_flag);

.

Suggestion:

fail_msg("Invalid value of -R parameter: %s\n", dynamic_flag);

---

src/common/tests/btree.c line 1073 at r3 (raw file):

			/* unknown parameter */
			break;
		}

It doesn't look right. Are you sure you need this despite the + at the beginning of the options string?
And how it was not necessary in the previous commit?

Code quote:

		if (opt == '?') {
			/* unknown parameter */
			break;
		}

---

src/common/tests/btree.sh line 114 at r3 (raw file):

        eval "${VCMD}" "$BTR"                       \
        --start-test "'btree functional ${test_conf_pre} ${test_conf} iterate=${IDIR}'" \
        -R"${DYN}" -M"${PMEM}" -C "${UINT}${IPL}o:$ORDER" \

It would be nice to either separate value from option with a space for all the options or none of them.

Code quote:

      -R"${DYN}" -M"${PMEM}" -C "${UINT}${IPL}o:$ORDER" \

---

src/common/tests/btree.sh line 134 at r3 (raw file):

        eval "${VCMD}" "$BTR" \
        --start-test "'btree batch operations ${test_conf_pre} ${test_conf}'" \
        -R"${DYN}" -M"${PMEM}" -C "${UINT}${IPL}o:$ORDER" \

.

---

src/common/tests/btree.sh line 143 at r3 (raw file):

        eval "${VCMD}" "$BTR" \
        --start-test "'btree drain ${test_conf_pre} ${test_conf}'" \
        -R"${DYN}" -M"${PMEM}" -C "${UINT}${IPL}o:$ORDER" \

.

---

src/common/tests/btree.sh line 150 at r3 (raw file):

        eval "${VCMD}" "$BTR" \
        --start-test "'btree performance ${test_conf_pre} ${test_conf}'" \
        -R"${DYN}" -M"${PMEM}" -C "${UINT}${IPL}o:$ORDER" \

.

---

src/common/tests/btree.c line 1061 at r2 (raw file):

	if (argc != optind) {
		print_message("Unknown parameter: %s\n", argv[optind]);
		return -1;

Suggestion:

		fail_msg("Unknown parameter: %s\n", argv[optind]);

---

src/common/tests/btree.c line 1078 at r2 (raw file):

	if (rc != 0) {
		print_message("daos_debug_init() failed: %d\n", rc);
		return rc;

Suggestion:

		fail_msg("daos_debug_init() failed: %d\n", rc);

[grom72]

Reviewed 1 of 2 files at r1, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @janekmi)

---

src/common/tests/btree.c line 1073 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Nope.

Done.

---

src/common/tests/btree.c line 916 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO it is a string not a character.

We expect character

---

src/common/tests/btree.c line 940 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

---

src/common/tests/btree.c line 1073 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It doesn't look right. Are you sure you need this despite the + at the beginning of the options string?
And how it was not necessary in the previous commit?

+ detects 'ma kotaas invalid parameter string but ignores-t`- it is not an error it is unknown parameter (but with proper syntax)

---

src/common/tests/btree.sh line 34 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please do it in the first commit.

Done

---

src/common/tests/btree.sh line 114 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It would be nice to either separate value from option with a space for all the options or none of them.

Done.

---

src/common/tests/btree.sh line 134 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/common/tests/btree.sh line 143 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/common/tests/btree.sh line 150 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/common/tests/btree.c line 1061 at r2 (raw file):

	if (argc != optind) {
		print_message("Unknown parameter: %s\n", argv[optind]);
		return -1;

let's not mix btree.c errors and test errors

---

src/common/tests/btree.c line 1078 at r2 (raw file):

	if (rc != 0) {
		print_message("daos_debug_init() failed: %d\n", rc);
		return rc;

.

---

src/common/tests/btree.c line 892 at r3 (raw file):

    {"start-test", required_argument, NULL, 'S'},
    {"reg_class", optional_argument, NULL, 'R'},
    {"reg_memory", optional_argument, NULL, 'M'},

see del_retain

[grom72]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 8 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[grom72]

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

[grom72]

Reviewed 45 of 45 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[grom72]

Reviewed 8 of 51 files at r8, all commit messages.
Reviewable status: 9 of 52 files reviewed, all discussions resolved (waiting on @grom72)

[janekmi]

Reviewed 1 of 8 files at r6, 2 of 45 files at r7, 9 of 51 files at r8, all commit messages.
Reviewable status: 15 of 52 files reviewed, 2 unresolved discussions (waiting on @grom72)

---

-- commits line 42 at r7:
Is it right?

Code quote:

  without space. Otherwise, the argument is incorectly interpreted
  as the next option (invalid one).

---

src/common/tests/btree.c line 978 at r8 (raw file):

			break;
		case 'M':
			ik_btr_memory_register(tst_fn_val.optval);

Either tst_fn_val is global or you pass its value as an argument.

Code quote:

			ik_btr_class_register(tst_fn_val.optval);
			break;
		case 'M':
			ik_btr_memory_register(tst_fn_val.optval);

[grom72]

Reviewable status: 15 of 52 files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

-- commits line 42 at r7:

Previously, janekmi (Jan Michalski) wrote…

Is it right?

Yes. The only solution to use space is to use ' e.g. '-M p'
I have updated commit message.

---

src/common/tests/btree.c line 978 at r8 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Either tst_fn_val is global or you pass its value as an argument.

Let's fix it separately.

[grom72]

Reviewed 43 of 51 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)