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.

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

libutil/read_all: fix minor bugs, add test coverage #1319

Merged
merged 4 commits into from Jan 17, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jan 16, 2018

libutil/readall.[ch] was misnamed, leaked on realloc failure, didn't preserve errno in all return paths, and lacked test coverage.

This PR addresses those issues and updates users where appropriate.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 16, 2018

Some of the travis builders failed to install GCC 4.9. I restarted one to see if it was a temporary failure...

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 16, 2018

Coverage Status

Coverage increased (+0.04%) to 78.579% when pulling ea523de on garlick:read_all_cleanup into 879ff60 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 16, 2018

Codecov Report

Merging #1319 into master will increase coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1319      +/-   ##
==========================================
+ Coverage   78.17%   78.19%   +0.01%     
==========================================
  Files         155      155              
  Lines       28198    28204       +6     
==========================================
+ Hits        22045    22054       +9     
+ Misses       6153     6150       -3
Impacted Files Coverage Δ
src/cmd/flux-module.c 84.45% <ø> (ø) ⬆️
src/cmd/builtin/content.c 73.87% <100%> (ø) ⬆️
src/cmd/flux-kvs.c 81.03% <100%> (ø) ⬆️
src/cmd/flux-logger.c 60% <100%> (ø) ⬆️
src/common/libutil/read_all.c 85.71% <83.33%> (ø)
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/common/libflux/message.c 81.25% <0%> (+0.11%) ⬆️
src/broker/overlay.c 74.2% <0%> (+0.31%) ⬆️
src/common/libutil/dirwalk.c 94.28% <0%> (+0.71%) ⬆️
src/broker/modservice.c 81.55% <0%> (+0.97%) ⬆️

garlick added some commits Jan 16, 2018

libutil/read_all: rename from readall.[ch]
Problem: readall.[ch] contains function named read_all().

Rename readall.[ch] to read_all.[ch].

Update users.
libutil/read_all: use void * buffers not uint8_t *
Problem: read_all() and write_all() use uint8_t *
buffer types that are less convenient than void *.

Update read_all() and write_all() prototypes to look
more like the read(2) and write(2) system calls they wrap.

Update callers where needed.
libutil/read_all: clean up error handling
Problem: read_all() leaks internally allocated buffer
upon realloc failure, and does not preserve errno in
all error paths.

Restructure realloc and error handling code to avoid
the leak and preserve errno.

@garlick garlick force-pushed the garlick:read_all_cleanup branch from ea523de to b57aa38 Jan 17, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 17, 2018

Coverage Status

Coverage increased (+0.01%) to 78.524% when pulling b57aa38 on garlick:read_all_cleanup into 877a6fd on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jan 17, 2018

Rebased on current master.

@grondo grondo merged commit c2e368e into flux-framework:master Jan 17, 2018

5 checks passed

buildbot/core_standard Build done.
Details
codecov/patch 85.71% of diff hit (target 78.17%)
Details
codecov/project 78.19% (+0.01%) compared to 877a6fd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 78.524%
Details
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 17, 2018

Looked good, merged.

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.