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

subprocess rework - minor fixes & enhancements #1629

Merged
merged 5 commits into from Aug 24, 2018

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Aug 24, 2018

Minor stuff I did / found along the way for issue #1331.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 24, 2018

Coverage Status

Coverage increased (+0.02%) to 79.529% when pulling ca70740 on chu11:misccleanup12 into 4a2674b on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #1629 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
+ Coverage   79.34%   79.36%   +0.01%     
==========================================
  Files         173      173              
  Lines       31885    31886       +1     
==========================================
+ Hits        25299    25305       +6     
+ Misses       6586     6581       -5
Impacted Files Coverage Δ
src/common/libflux/buffer.c 95.55% <100%> (+0.03%) ⬆️
src/broker/broker.c 76.77% <100%> (ø) ⬆️
src/modules/cron/task.c 81.74% <100%> (-0.08%) ⬇️
src/common/libflux/content.c 86.66% <0%> (-3.34%) ⬇️
src/common/libflux/rpc.c 92.68% <0%> (-0.82%) ⬇️
src/common/libflux/response.c 82.89% <0%> (-0.66%) ⬇️
src/bindings/lua/flux-lua.c 82.15% <0%> (-0.09%) ⬇️
src/common/libflux/message.c 80.78% <0%> (+0.11%) ⬆️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/cmd/flux-module.c 85.36% <0%> (+0.3%) ⬆️
... and 4 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 24, 2018

LGTM - except, is it possible to overrun the buffer when adding the null terminators in the first commit?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 24, 2018

Nope, the buffer is created with "+1" length for this purpose, so it's safe.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 24, 2018

Thanks for clarifying. Maybe a quick ack from @grondo and then we can merge?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 24, 2018

Looks fine to me! Merging...

@grondo grondo merged commit 72a0550 into flux-framework:master Aug 24, 2018

4 checks passed

codecov/patch 100% of diff hit (target 79.34%)
Details
codecov/project 79.36% (+0.01%) compared to 4a2674b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 79.529%
Details
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.