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

libflux/flog: minor fixes and cleanup #1939

merged 7 commits into from Jan 21, 2019


Copy link

commented Jan 21, 2019

This PR drops a logging option FLUX_LOG_CHECK, which makes flux_log() into a synchronous RPC. It seems to only be used in flux-logger(1), and there (as far as I can tell) only so that a sharness security test can verify that flux_log() receives an EPERM response for non instance owner. Rework that test and then drop the option.

In addition, flux_log() calls flux_rpc(FLUX_RPC_NORESPONSE) which creates a future and then immediately destroys it. Replace this with a direct flux_send() of the request message to make logging a bit less malloc heavy, now that the FLUX_LOG_CHECK option isn't supported.

Finally, ensure that flux_log() always returns 0 on success, as documented in its manual page, and add a test for logging with the flux_t handle set to NULL.

garlick added 7 commits Jan 20, 2019
Problem: flux-logger is using the FLUX_LOG_CHECK option
presumably just for the benefit of a sharness security
test, which has been reworked not to need it.

Drop the use of this flag.
Problem: FLUX_LOG_CHECK option for logging with a synchronous
RPC is no longer needed.

Drop this option.
Problem: security sharness assumes flux-logger fails
with EPERM for non instance owner, but this depends
on the FLUX_LOG_CHECK flag which is going away.

Alter the test to check if the proposed log message
is present in the ring buffer using flux dmesg, rather
than checking that the flux logger command itself failed.

Also some minor clenaup:
- drop trailing whitespace
- drop extraneous MSG definition in dmesg security test
Problem: flux_log() calls flux_rpc(FLUX_RPC_NORESPONSE),
which creates a future that is immediately destroyed.

To make logging a bit more lightweight, simply allocate a
request message and send it.
Problem: flux_log() is documented to return -1 or 0,
but when h=NULL, it returns the number of characters

Ensure it returns 0 on success in all cases.
Problem: unit test doesn't call flux_log with flux_t handle
set to NULL, but code exists to direct the log to stderr
when there is no broker connection.

Add a test case for this.
Drop the documentation for FLUX_LOG_CHECK,
which is being removed.

This comment has been minimized.

Copy link

commented Jan 21, 2019

Codecov Report

Merging #1939 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1939      +/-   ##
- Coverage    80.1%   80.07%   -0.04%     
  Files         195      195              
  Lines       35066    35059       -7     
- Hits        28091    28074      -17     
- Misses       6975     6985      +10
Impacted Files Coverage Δ
src/cmd/flux-logger.c 61.76% <100%> (-2.95%) ⬇️
src/common/libflux/flog.c 90.97% <100%> (-0.46%) ⬇️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/broker/modservice.c 78.84% <0%> (-0.97%) ⬇️
src/modules/connector-local/local.c 74.81% <0%> (-0.89%) ⬇️
src/broker/log.c 71.73% <0%> (-0.31%) ⬇️
src/common/libflux/message.c 81.39% <0%> (-0.13%) ⬇️
src/modules/kvs/kvs.c 66.92% <0%> (+0.14%) ⬆️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️

This comment has been minimized.

Copy link

commented Jan 21, 2019

Seems like good cleanup? Ready to merge?


This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

sure, thanks

@grondo grondo merged commit 2d647ec into flux-framework:master Jan 21, 2019
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 80.1%)
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +19.89% compared to f5ead4b
continuous-integration/travis-ci/pr The Travis CI build passed
@garlick garlick deleted the garlick:log_cleanup branch Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
3 participants
You can’t perform that action at this time.