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

librouter: move sendfd, recvfd out of public API #2375

Merged
merged 11 commits into from Sep 15, 2019

Conversation

@garlick
Copy link
Member

commented Sep 14, 2019

Another bit peeled off of #2303 (librouter). This one removes some specialized functions for sending and receiving flux messages over file descriptors from the main API, renaming it and relocating to the private librouter library (which this PR creates). Convert users, add inline documentation, and enhance unit testing.

garlick added 9 commits Aug 29, 2019
Problem: flux_msg_sendfd() et al, and the "iobuf" struct
are low level tools for transporting flux messages over
file descriptors that have very rough API's and do not need
to be included in the public Flux API.

As a first step, duplicate flux_msg_sendfd() et al from
libflux to the librouter internal library and rename:

flux_msg_sendfd() -> sendfd()
flux_msg_recvfd() -> recvfd()
struct flux_msg_iobuf -> struct iobuf
flux_msg_iobuf_init() -> iobuf_init()
flux_msg_iobuf_clean() -> iobuf_clean()

Duplicate unit tests from libflux/test/message.c.

Next steps will be to convert users, then remove public defs.
Problem: sendfd() and recvfd() segfault if presented
with invalid arguments.

Bad args are detected, but teardown attempts to free storage
in uninitialized internal iobuf.  Fix the code to return
immediately rather than executing the teardown.
Use BAIL_OUT() to handle errors in stuff not being tested,
so that ok() output shows actual test results.

Add unit tests for sendfd/recvfd that send and receive
many messages over a non-blocking pipe, so that the iobuf
partial message buffering has coverage.

Add coverage for invalid args.
Drop flux_msg_sendfd/recvfd() and flux_msg_iobuf from the
public API now that functionality has been replicated in
the internal librouter library.

Drop unit tests that have also been replicated.
Now that flux_msg_senfd() has been privatized
in librouter, drop its public man page.
@garlick garlick force-pushed the garlick:librouter_sendfd branch from 7d53266 to 81b1f27 Sep 14, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2019

Rebased on current master, and noting missing coverage of bad args, added some to the unit tests and discovered a segfault, which was fixed.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2019

Restarted a clang builder that failed here (I assume just a spurious timeout)

ERROR: t1003-kvs-stress.t - exited with status 137 (terminated by signal 9?)
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2019

Hmm, builds finished successfully, but travis-ci completion didn't seem to get picked up by github. I went ahead and restarted the whole travis-ci build.

garlick added 2 commits Sep 14, 2019
Problem: if malloc fails in recvfd(), an errno of EPROTO
is used.

Don't disturb errno value of ENOMEM already set by malloc()
on failure.
Ensure that recvfd() fails with ECONNRESET when read(2)
internally returns EOF, as advertised.
@codecov-io

This comment has been minimized.

Copy link

commented Sep 14, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@e7c28a2). Click here to learn what that means.
The diff coverage is 92.23%.

@@            Coverage Diff            @@
##             master    #2375   +/-   ##
=========================================
  Coverage          ?   80.85%           
=========================================
  Files             ?      220           
  Lines             ?    34594           
  Branches          ?        0           
=========================================
  Hits              ?    27971           
  Misses            ?     6623           
  Partials          ?        0
Impacted Files Coverage Δ
src/common/libflux/message.c 80.64% <ø> (ø)
src/connectors/ssh/ssh.c 85.9% <100%> (ø)
src/connectors/local/local.c 89.28% <100%> (ø)
src/cmd/builtin/proxy.c 74.12% <85.71%> (ø)
src/modules/connector-local/local.c 73.11% <85.71%> (ø)
src/common/librouter/sendfd.c 92.2% <92.2%> (ø)
@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

LGTM! My only comment is that the naming of sendfd recvfd and iobuf_* are pretty generic, and don't necessarily look like they'd be something in the "librouter" convenience library. However, I don't really have a better suggestion at this time and I don't feel strongly enough at this time to hold up the PR.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

Yeah, I wasn't sure about that, but I also didn't have a great alternate name on the tip of my tongue. It isn't published, and it won't have broad usage within flux-core, so could be renamed later if we found a problem. This is prob OK to go in if you think it looks OK.

@grondo grondo merged commit a7e2a6d into flux-framework:master Sep 15, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

Thanks!

@garlick garlick deleted the garlick:librouter_sendfd branch Sep 16, 2019
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.