Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAOS-16953 tests: fix btree parameter parsing #15850

Merged
merged 4 commits into from
Feb 12, 2025
Merged

DAOS-16953 tests: fix btree parameter parsing #15850

merged 4 commits into from
Feb 12, 2025

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Feb 5, 2025 •
edited
Loading

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.

All parameters parsing process has been unified.

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

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

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.
    NOTE: Functional Tests and HW Tests are intentionally skipped as they do not use btree/btree_direct binaries.

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.

@grom72 grom72 requested a review from a team as a code owner February 5, 2025 16:04
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2025 •
edited
Loading

Ticket title is 'btree.sh argument parsing is off'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-16953

@grom72 grom72 requested review from jolivier23 and rpadma2 February 5, 2025 16:05
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/1/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/1/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/5/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/5/testReport/

@grom72 grom72 marked this pull request as draft February 7, 2025 07:59
@grom72 grom72 changed the title DAOS-16953 tests: fix btree parameter parsing DAOS-16953 tests: fix btree parameter parsing [WiP] Feb 7, 2025
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/6/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/6/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/8/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/8/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15850/9/testReport/

@grom72
DAOS-16953 tests: fix btree parameter parsing
c1c998b 
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 test
(argc/argv) and avoid static variables for that purpose.

Co-authored-by: Jan Michalski <[email protected]>
Co-authored-by: Tomasz Gromadzki <[email protected]>

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72
Introduce new parameters for memory and class reg.
a8dd381 
Introduce new parameters for explicity class and memory
registration. Both parameters must be provided before
`-C` parameter.

Syntax is as follow:
* -M[p] for memory registration. The `p` value inidcates
  the usage of persisten memory
* -R[d] for class registration. The `d` value indicates
  the usage of `BTR_FEAT_DIRECT_KEY` flag - root is
  dynamically sized up to tree order.

Parameters `-t` and `-d` are no longer valid.

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72
No space before optional option's argument
c81cb9b 
-R and -M options have optional argument.
The optional argument must be specified on the command line
without space. Otherwise, the argument is incorectly interpreted
as the next option (invalid one).

From getopt() documentation:
about option with mandatory argument:
"optstring is a string containing the legitimate option characters.
If such a character is followed by a colon, the option requires
an argument, so getopt() places a pointer to the following text
in the same argv-element, or the text of the following argv-element,
in optarg."
about option with optional argument:
"Two colons mean an option takes an optional arg; if there is text
in the current argv-element (i.e., in the same word as the option
name itself, for example, "-oarg"), then it is returned in optarg,
otherwise optarg is set to zero."

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72
Adapt btree_direct.c to follow btree.c
5f848f0 
btree_direct.c and btree.c must follow the same parameters
except dyenamic registration and drain.

Unify test execution implementation.

Limit validation scope:
Skip-func-hw-test: true
Skip-func-test: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72 grom72 changed the title DAOS-16953 tests: fix btree parameter parsing [WiP] DAOS-16953 tests: fix btree parameter parsing Feb 10, 2025
@grom72 grom72 requested a review from jolivier23 February 10, 2025 15:12
@grom72 grom72 marked this pull request as ready for review February 10, 2025 15:13
@grom72 grom72 requested a review from NiuYawei February 10, 2025 15:26
@grom72
Copy link
Contributor Author

grom72 commented Feb 12, 2025

All required tests passed.
https://build.hpdd.intel.com/job/daos-stack/job/daos/view/change-requests/job/PR-15850/11/testReport/
Neither Functional nor HW tests are in the scope of this change.

@grom72 grom72 requested a review from a team February 12, 2025 06:56
@jolivier23 jolivier23 merged commit 7c475ac into master Feb 12, 2025
59 checks passed
@jolivier23 jolivier23 deleted the grom72/daos-16953 branch February 12, 2025 15:42
grom72 added a commit that referenced this pull request Feb 13, 2025
@grom72
DAOS-16953 tests: fix btree parameter parsing (#15850)
a94e485 
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.

All parameters parsing process has been unified.

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
struct btr_test_state introduced to provide global parameters to the test (argc/argv) and avoid static variables for that purpose.

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72 grom72 mentioned this pull request Feb 13, 2025
Open
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiuYawei NiuYawei NiuYawei approved these changes

@jolivier23 jolivier23 jolivier23 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grom72 @daosbuild1 @NiuYawei @jolivier23