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

cleanup of flux_msg_copy(), flux_rpc_aux_set() etc. and tests #1056

Merged
merged 11 commits into from May 11, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 11, 2017

This is some cleanup peeled off of PR #1055.

Relatively minor stuff: adding aux get/set functions to flux_rpc_t and flux_msg_t,
making flux_msg_copy() more efficient, and adding tests covering these changes.

@garlick garlick force-pushed the garlick:libflux_cleanup branch from ad3f27b to 0a2de52 May 11, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 11, 2017

Codecov Report

Merging #1056 into master will decrease coverage by 0.01%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master    #1056      +/-   ##
==========================================
- Coverage   77.68%   77.67%   -0.02%     
==========================================
  Files         148      148              
  Lines       25752    25787      +35     
==========================================
+ Hits        20006    20030      +24     
- Misses       5746     5757      +11
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 80.29% <100%> (-0.44%) ⬇️
src/common/libflux/dispatch.c 83.24% <100%> (-0.28%) ⬇️
src/broker/content-cache.c 72.54% <50%> (-0.64%) ⬇️
src/cmd/flux-ping.c 86.86% <66.66%> (-0.64%) ⬇️
src/common/libflux/rpc.c 90.9% <75%> (-0.3%) ⬇️
src/common/libflux/message.c 81.66% <83.33%> (+0.7%) ⬆️
src/common/libflux/response.c 83.76% <0%> (-0.86%) ⬇️
... and 3 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2017

Coverage Status

Coverage decreased (-0.002%) to 77.933% when pulling ad3f27b on garlick:libflux_cleanup into 095a509 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2017

Coverage Status

Coverage decreased (-0.01%) to 77.921% when pulling 0a2de52 on garlick:libflux_cleanup into 095a509 on flux-framework:master.

garlick added some commits May 3, 2017

libflux/rpc: use a hash for flux_rpc_aux_get/set
Problem: flux_rpc_t allows at most one auxiliary data member
to be stored in the object.

Solution: add a zhash internally and allow data members to be
added by name.  The interface is now identical to the one
on flux_t handles, except that flux_rpc_aux_set() can return an
error on malloc failure, while flux_aux_set() is typed void.

Update users in kvs, content-cache, and flux-ping.
libflux/rpc: drop unused flux_rpc_type_get/set
Problem: flux_rpc_type_get/set are not used and don't
seem to be useful.

These interfaces were added as a way to "extend" the
flux_rpc_t type for other operations.  However, more
sophisticated ideas for polymorphism have been proposed,
and it's probably best to just drop this.
libflux/message: add flux_msg_aux_get/set
Allow aux data to be associated with a message object.

The main use case for this is to allow message parsing
functions to return memory "owned" by the message, that will
be destroyed when the message is destroyed, so the user
doesn't have to manage that allocation separately.

Since message handlers receive a (const flux_msg_t *) parameter,
the design of flux_msg_aux_set() rationalizes taking a const
message and modifying it anyway with the argument that this
is an "annotation" that doesn't actually change the content
of the message.
libflux/types: move flux_free_f def to types.h
Problem: the flux_free_f typedef is defined in handle.h
and thus cannot be referenced in a header included by
handle.h, such as message.h.

Move flux_free_f def to a new header, types.h,
which can be included by handle.h and other headers
as needed.
libflux/message: make flux_msg_copy() more efficient
Problem: flux_msg_copy() accepts a flag indicating whether or
not the payload should be copied, however it would copy it
anyway and then delete it if the flag was present.

Copy frame by frame and avoid the payload duplication
when possible.
libflux/message: drop flux_msg_has_route()
Problem: flux_msg_has_route() is unused and has no test coverage.

Drop this function.

@garlick garlick force-pushed the garlick:libflux_cleanup branch from 0a2de52 to 0ab4ef1 May 11, 2017

@garlick garlick force-pushed the garlick:libflux_cleanup branch from 0ab4ef1 to 5a6ca1f May 11, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2017

Coverage Status

Coverage decreased (-0.01%) to 77.932% when pulling 5a6ca1f on garlick:libflux_cleanup into 79e983a on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 11, 2017

Looks good, just waiting for last Travis check to pass. Is it ready to merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 11, 2017

Yes ready.

@grondo grondo merged commit 5d36372 into flux-framework:master May 11, 2017

2 of 4 checks passed

codecov/patch 77.27% of diff hit (target 77.68%)
Details
codecov/project 77.67% (-0.02%) compared to 79e983a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 77.932%
Details

@garlick garlick deleted the garlick:libflux_cleanup branch May 11, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.