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

cron: avoid use of json-c and xzmalloc #1143

Merged
merged 16 commits into from Aug 16, 2017

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented Aug 4, 2017

This PR updates the cron module to use jansson instead of json-c, removes other uses of xzmalloc that would result in calls to oom() and friends, and fixes some other leaks and issues found along the way.

A new function for was added to the cronodate interface to allow adding non-range integers to a cronodate object, for cases where arguments to cron "datetime" entries are encoded by the caller as numbers and not strings (as is done for ranges, e.g. "0-3" or glob "*"). The datetime argument parser was then updated to check for whether individual arguments were encoded as strings or numbers, and uses the appropriate function cronodate_set or cronodate_set_integer. This is required because unlike json-c, jansson doesn't allow decoding a number value as a string.

This PR is based on top of #1142, becuase otherwise I couldn't get successful builds in my own branch. I'll rebase if that PR gets merged.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 5, 2017

Coverage Status

Coverage decreased (-0.1%) to 77.812% when pulling ca179c3 on grondo:cron-jansson into 38fed2d on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 5, 2017

Codecov Report

Merging #1143 into master will increase coverage by 0.02%.
The diff coverage is 78.92%.

@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
+ Coverage   77.55%   77.57%   +0.02%     
==========================================
  Files         157      157              
  Lines       28699    28756      +57     
==========================================
+ Hits        22257    22308      +51     
- Misses       6442     6448       +6
Impacted Files Coverage Δ
src/common/libutil/shortjson.h 91.46% <ø> (+0.65%) ⬆️
src/common/libutil/cronodate.c 94.51% <100%> (+0.31%) ⬆️
src/modules/cron/datetime.c 90.62% <100%> (+1.33%) ⬆️
src/modules/cron/event.c 81.08% <67.85%> (-7.97%) ⬇️
src/modules/cron/task.c 80.16% <72.83%> (-4.87%) ⬇️
src/modules/cron/interval.c 82.14% <77.77%> (-7.52%) ⬇️
src/modules/cron/cron.c 79.48% <80.18%> (-1.95%) ⬇️
src/common/libkvs/jansson_dirent.c 64.44% <0%> (-2.23%) ⬇️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/common/libflux/message.c 81.29% <0%> (ø) ⬆️
... and 16 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 6, 2017

This is excellent cleanup. I'll give it a bit more thorough review on monday, but looks good so far.

I was going to say something about calloc/strdup not setting errno, but according to the man page they do per UNIX98! Ugh, I had been assuming they didn't. I will need to mend my ways and fix some code.

@grondo grondo force-pushed the grondo:cron-jansson branch 2 times, most recently from 437a4b1 to ed1d105 Aug 7, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 7, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.797% when pulling 437a4b1 on grondo:cron-jansson into 0abcd67 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Aug 7, 2017

I was going to say something about calloc/strdup not setting errno, but according to the man page they do per UNIX98! Ugh, I had been assuming they didn't. I will need to mend my ways and fix some code.

Well you caught me, I just assumed they set errno appropriately. Thanks for checking on it.

I've pushed a rebase on current master. Also, I noticed that the cron module appeared to be the last user of Jadd_bool so I've added a commit to remove that function.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Aug 7, 2017

Hm, one build failed with a write error during the very final phase of distcheck. I've restarted it since the other builds were successful.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 7, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.812% when pulling 9598b9e on grondo:cron-jansson into 0abcd67 on flux-framework:master.

@grondo grondo force-pushed the grondo:cron-jansson branch from 9598b9e to 6f256e5 Aug 8, 2017

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 8, 2017

One other review comment since at one point this came up in discussion: the jansson unpack s?type notation (optional key) used here is jansson 2.3 or newer. We require 2.6 or newer so this is fine.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.81% when pulling 6f256e5 on grondo:cron-jansson into 0abcd67 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Aug 8, 2017

One other review comment since at one point this came up in discussion: the jansson unpack s?type notation (optional key) used here is jansson 2.3 or newer. We require 2.6 or newer so this is fine.

Thanks for double checking on that.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 9, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.963% when pulling 154aa05 on grondo:cron-jansson into 0abcd67 on flux-framework:master.

@grondo grondo force-pushed the grondo:cron-jansson branch from 154aa05 to 44f40eb Aug 10, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 10, 2017

Coverage Status

Coverage decreased (-0.1%) to 77.854% when pulling 44f40eb on grondo:cron-jansson into 1665912 on flux-framework:master.

@grondo grondo force-pushed the grondo:cron-jansson branch from 44f40eb to 62f0052 Aug 14, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-0.07%) to 77.88% when pulling 62f0052 on grondo:cron-jansson into aeb6cfd on flux-framework:master.

grondo added some commits Aug 4, 2017

libutil/cronodate: add cronodate_set_integer()
Add a function to set a single integer value for any time unit
member of a cronodate object. This is a convenience function to
prevent requiring a single integer to be converted to a string
and passed in as a range, which is then converted right back to
an integer.
libutil/test: add tests for cronodate_set_integer
Test new API function cronodate_set_integer().

@grondo grondo force-pushed the grondo:cron-jansson branch from 62f0052 to 5f3e064 Aug 14, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-0.04%) to 77.884% when pulling 5f3e064 on grondo:cron-jansson into fc2a4ff on flux-framework:master.

grondo added some commits Aug 2, 2017

modules/cron: drop xzmalloc and xstrdup
Remove calls to xzmalloc() and xstrdup in favor of straight
malloc(). Return errors to caller (when possible) instead of
using oom() etc to abort when memory allocations fail.
modules/cron: switch to jansson
Drop use of json-c and shortjson.h in cron module in favor of jansson.
flux-cron: ensure some msg fields are encoded as numbers
Promote arguments and options that should be encoded as numbers in
JSON usint tonumber() where appropriate. Jansson seems to be less
forgiving about encoding numbers as strings when parsing message
payloads.
modules/cron: fix leak in cron_entry_destroy
Fix missing free of `typename` member of cron entry object.
Also, remove unnecessary const qualifier on this member.
modules/cron: update datetime to allow numeric args
Allow datetime 'ranges' to be numeric arguments as well as
string ranges in the cron datetime handler. This is required
because the lua frontend flux-cron(1) utility always encodes
numbers that are integers as JSON ints.
modules/cron: avoid implicit rounding of timestamps
Avoid implicit rounding in timespec_to_double in both cron.c and task.c.

For the cron task timestamps, rename the local function
round_timespec_to_double() to be honest about what we're doing,
and expand the comment to explain why.

For other cron timestamps, just drop the rounding for now. If it
an inconvenience in the future (unlikely), the rounding can be
copied from task.c as an example.

Resolves #1131
modules/cron: pass env as json_t in cron_task_run
For efficiency, make cron_task_run() take a json_t object instead
of a char ** environment array. This will allow cron entry to store
the optional environment array as a JSON object, and avoid conversion
from JSON to character array and back again.

grondo added some commits Aug 8, 2017

modules/cron: support cwd and environ in cron.create
Allow optional cwd and environment to be passed in during cron.create,
and store these as part of the cron entry, to be sent along with each
cron task invocation.
flux-cron: add options to preserve env and set working dir
Add --preserve-env and --working-dir=DIR options to send current
environment as cron task environment, and DIR as current working
directory.
doc: update flux-cron manpage
Add documentation for the --preserve-env and --working-dir options
to the 'event' 'interval' 'tab' and 'at' subcommands.
t0015-cron.t: add tests for new options
Add tests for --preserve-env and --working-dir options.
t0015-cron.t: enhance cron sync test
Enhance check for cron module loaded with sync enabled.
Ensure both sync event and sync_epsilon match what was
passed on the cmdline.
libutil/shortjson.h: remove Jadd_bool
Jadd_bool no longer has any users. Remove it.
travis-ci: disable romio on mpich build
To save time, add --disable-romio to mpich build in travis-ci.

@grondo grondo force-pushed the grondo:cron-jansson branch from 5f3e064 to be2bfe2 Aug 15, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 15, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.905% when pulling be2bfe2 on grondo:cron-jansson into fc2a4ff on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 15, 2017

Is this one ready?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Aug 15, 2017

Yes, unless there was more someone wanted me to do...

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 16, 2017

LGTM.

@garlick garlick merged commit 8e439c6 into flux-framework:master Aug 16, 2017

4 checks passed

codecov/patch 78.92% of diff hit (target 77.55%)
Details
codecov/project 77.57% (+0.02%) compared to fc2a4ff
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 77.905%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@grondo grondo deleted the grondo:cron-jansson branch Oct 25, 2017

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.