libpmi: cleanup old code and optimize client reads #5423
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: flux-start includes dgetline.h but doesn't use it Drop include directive.
Problem: the libpmi "simple client" reads single characters from the server socket in order to build a line without the need to buffer characters. Although this approach is functional, it is a best practice to minimize the number of read(2) calls using buffering when possible. Use libc buffered I/O instead.
Problem: dgetline.c is only used by libpmi tests now. Wean tests off of dgetline.c and drop it: - use a flux buffer watcher in the test server - use stdio buffered I/O in the test clients
Problem: some older code in libpmi is formatted in a way that clashes with the modern Flux code base. Adjustments: - make long parameter lists one per line - make long conditional lists one condition per line (1 indent) - declare pointers with no space between '*' and name - use '*' to declare simple pointer parameters not the equivalent but less clear '[]' notation inherited from the canonical Argonne headers
Problem: PMI2_ERR_OTHER is the only PMI2 error not decoded by pmi_strerror(). The PMI-1 and PMI-2 error codes are all the same except PMI-2 adds PMI2_ERR_OTHER. Since Flux can bootstrap from libpmi2, these errors can be seen by Flux in real life. Add PMI2_ERR_OTHER to the error string table.
garlick
changed the title
garlick
force-pushed
the
libpmi_stdio
branch
3 times, most recently
from
872a88e
to
88421c1
Compare
Problem: strlcpy() is a handy function that is not widely available on linux. Add openbsd strlcpy() to the internal libutil convenience library.
Problem: github CodeQL calls out an unbounded write in libpmi/pmi2.c. The bounds are actually checked but this would be clearer and possibly easier for static analysis to understand with strlcpy(3). Use strlcpy(3).
garlick
force-pushed
the
libpmi_stdio
branch
from
88421c1
to
224d82d
Compare
Problem: github CodeQL calls out a missed overflow check in users of keyval_parse_uint(). keyval_parse_uint() and keyval_parse_int() assign the result of strtoul() and strtol() respectively without checking for under/ overflow. Add checks. Update unit tests.
garlick
force-pushed
the
libpmi_stdio
branch
from
224d82d
to
66f9d71
Compare
|
Thanks! Just repushed with one of the commit messages slightly improved. I'll set MWP. |
garlick
changed the title
Codecov Report
@@ Coverage Diff @@
## master #5423 +/- ##
==========================================
- Coverage 83.61% 83.61% -0.01%
==========================================
Files 480 480
Lines 80560 80565 +5
==========================================
+ Hits 67359 67362 +3
- Misses 13201 13203 +2
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is some gratuitous cleanup of old code in libpmi. The main change is replacing the internal
dgetline()function with buffered stdio in the PMI client, which substantially reduces the number ofread(2)calls. E.g. just a little strace snippet from the tail end of a simple PMI conversation, before and after:master
libpmi_stdio
I don't think that's actually going to affect performance much since the collective exchange operation dominates in a typical PMI session, but...blech! At least it reduces the gag reflex.