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

check for invalid FP types in fsd #2216

Merged
merged 1 commit into from Jul 2, 2019

Conversation

@trws
Copy link
Member

commented Jul 1, 2019

Adding a check to disallow zeros, NaNs and infinites in flux standard
duration format parsing in fsd. This came up while looking into #2213.

Just to see if we actually had issues with this, I tried a broker with shutdown grace duration of NaNs, which hangs and has to be killed with signal 9 to die so, unlikely as this may be, may as well head it off.

This probably needs an update in the RFC as well, but thought we could work it out here first.

@trws trws requested review from garlick, grondo and SteVwonder Jul 1, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Thanks! Looks like the unit test (libutil/test/fsd.c) needs an update to pass a non-zero value for these tests that are expected to pass (Hmm, makes me wonder if @grondo had a use case for zero in mind?)

FAIL: test_fsd.t 7 - fsd_parse_duration (0) returns success
FAIL: test_fsd.t 9 - fsd_parse_duration (0s) returns success
FAIL: test_fsd.t 11 - fsd_parse_duration (0m) returns success
FAIL: test_fsd.t 13 - fsd_parse_duration (0h) returns success
FAIL: test_fsd.t 15 - fsd_parse_duration (0d) returns success

and while in there, it would be good to add expected failures for the new bad values.

We'll need a PR on https://github.com/flux-framework/rfc/blob/master/spec_23.adoc as well.

@trws

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Good point, I should have thought to add the test up front. I guess I'm ok with leaving zero ok if we have a use for that, maybe it's meaningful for something, or a key for "forever"?

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Maybe leave zero valid for now and we can fix it later if it's a loose end.

@SteVwonder

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Hmm, makes me wonder if @grondo had a use case for zero in mind?

maybe it's meaningful for something, or a key for "forever"?

Yeah, RFC14 defines a duration of 0 in jobspec to mean unlimited or not set by the system.

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

One builder failed in the kvs namespace test (looks like broker killed with -9). Appears unrelated, so I restarted.

@trws if you could squash the incremental development this can go in IMHO.

(fine to just squash down to one commit I think).

@trws

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

Adding a check to disallow NaNs and infinites in flux standard
date format parsing and serialization in fsd.  This came up while
looking into #2213.

Also added a test for EINVAL on "NaNs" and "infinites", this
also covers the various other forms of those, but I didn't see a reason
to test strtod.
@trws trws force-pushed the trws:check_invalid_float branch from c1cd708 to b94a751 Jul 2, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

LGTM thanks!

@garlick garlick merged commit a2a44e1 into flux-framework:master Jul 2, 2019
2 checks passed
2 checks passed
Summary 1 potential rule
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.