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

avoid use of xzmalloc and oom in libflux API sources #1060

Merged
merged 16 commits into from May 15, 2017

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented May 13, 2017

This was a result of a quick pass through the libflux source files removing the use of functions that call oom(). There might be more work needed here, and I'm not sure in the end if this actually accomplishes very much because other functions in libutil that are called from libflux might end up calling oom() as well.

There may be some better solutions for error handling is some cases as well, but I'm just throwing up this PR for feedback.

(This was part of the dirwalk PR and I've just split it out, so it might not even pass travis at this point)

@garlick
Copy link
Member

garlick left a comment

Commit message s/ro/so/.

What about passing size of impl instead of the impl pointer into flux_watcher_create(), and using offsetof to tackle the allocation in one calloc call (e.g. allocate offsetof (struct flux_watcher, impl) + impl_size)?

(I think that's safe for impl alignment...er, if not, then strike that idea)

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 13, 2017

Ooops, I meant that last comment to be a targetted review comment on the reactor changes, not the entire review, sorry.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 13, 2017

Just wondering if maybe the log functions should call FLUX_FATAL(h) internally on ENOMEM or other I/O error? (see examples in handle.c - functions by default return errors directly, but they can call the user's noreturn error function before returning if the user registered one and the handle is unlikely to be usable afterwards).

typedef void (*flux_fatal_f)(const char *msg, void *arg);

/* Register a handler for fatal handle errors.
 * A fatal error is ENOMEM or a handle send/recv error after which
 * it is inadvisable to continue using the handle.
 */
void flux_fatal_set (flux_t *h, flux_fatal_f fun, void *arg);

/* Set the fatality bit and call the user's fatal error handler, if any.
 * The fatal error handler will only be called once.
 */
void flux_fatal_error (flux_t *h, const char *fun, const char *msg);
#define FLUX_FATAL(h) do { \
    flux_fatal_error((h),__FUNCTION__,(strerror (errno))); \
} while (0)

/* Return true if the handle 'h' has encountered a fatal error.
 */
bool flux_fatality (flux_t *h);

grondo added some commits May 4, 2017

libflux/reactor: avoid use of xzmalloc and oom
xzmalloc() calls oom() on failure which triggers and exit of
the calling program. This should not be done in libflux context,
ro replace the calls to xzmalloc() with calloc() and manually
check return codes.

Instead of repeatedly calling the same sequence of error handling
when flux_watcher_create() fails (free impl, set errno, return), a
convenience function was added `flux_watcher_create_cleanup`,
which handles cleanup, so that a single return can be used on
failure.
libflux/attr: avoid use of xzmalloc and oom
libflux API interfaces should avoid use of oom and xzmalloc,
which cause unexpected exit of caller.
libflux/flog: avoid use of xzmalloc and oom
Avoid use of convenience functions that call oom() on failure from
libflux API entry points.
libflux/flog: possible use of wrong errno in flux_log_error
Fix possible use of incorrect errno in flux_vlog_error. In the case
that vasprintf() resets or changes errno, then we probably want to
pass saved_errno to flux_strerror() not errno.
libflux/module: avoid use of xzmalloc and oom
Avoid the use of xzmalloc and oom in libflux API interfaces
libflux/response: remove xzmalloc.h
No xzmalloc functions are used in libflux/response.c, remove the header file.
libflux/reduce: avoid use of xzmalloc and oom
Do not use xzmalloc and oom() in libflux API functions.
libflux/reparent: avoid use of xzmalloc functions
Remove use of xstrdup in libflux/reparent.

@grondo grondo force-pushed the grondo:cleanups branch from e955b6d to d08be4c May 14, 2017

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 14, 2017

Rebased on master.

Great suggestions. I might not get back to this until Monday, but feel free to push to this PR if you have any desire to clean it up a bit in the meantime. ;-)

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.974% when pulling d08be4c on grondo:cleanups into 9c78848 on flux-framework:master.

libflux/reactor: optimize flux_watcher_create()
Allocate space for specific ev watcher plus flux_watcher_t
in one calloc() call.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 14, 2017

Codecov Report

Merging #1060 into master will increase coverage by 0.05%.
The diff coverage is 81.35%.

@@            Coverage Diff             @@
##           master    #1060      +/-   ##
==========================================
+ Coverage   77.73%   77.79%   +0.05%     
==========================================
  Files         149      148       -1     
  Lines       25865    25771      -94     
==========================================
- Hits        20107    20049      -58     
+ Misses       5758     5722      -36
Impacted Files Coverage Δ
src/common/libflux/response.c 84.61% <ø> (ø) ⬆️
src/broker/broker.c 72.58% <100%> (+0.75%) ⬆️
src/common/libflux/message.c 81.56% <100%> (+0.02%) ⬆️
src/common/libflux/reduce.c 79.16% <60%> (-0.42%) ⬇️
src/common/libflux/attr.c 90.81% <68.18%> (-3.69%) ⬇️
src/common/libflux/module.c 77.83% <77.77%> (-0.17%) ⬇️
src/common/libflux/flog.c 92.98% <80.95%> (-2.35%) ⬇️
src/broker/overlay.c 71.32% <85.71%> (+4.55%) ⬆️
src/common/libflux/reactor.c 94.89% <91.66%> (-1.34%) ⬇️
src/cmd/flux-comms.c 56.66% <91.66%> (+10.37%) ⬆️
... and 5 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.06%) to 77.939% when pulling 7434220 on grondo:cleanups into 9c78848 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 14, 2017

Pushed one commit that reworks flux_watcher_create for one malloc instead of two. See what you think?

libflux/reactor: optimize flux_periodic_watcher_create
Create f_periodic wrapper struct as part of the flux_watcher_t
and disable the destructor, so they look like the other watchers.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.1%) to 77.887% when pulling fd504fa on grondo:cleanups into 9c78848 on flux-framework:master.

garlick added some commits May 14, 2017

libflux/flog: call FLUX_FATAL from flux_vlog()
If an error occurs in flux_vlog(), call FLUX_FATAL() in
addition to returning -1.  In practice, log calls will
rarely be checked and registering a fatal error handler
may be the best alternative to detect errors here.
libflux/flog: avoid malloc in flux_log_verror()
Since the max size of a log message is known,
flux_log_verror() can call vsnprintf() on the
stack to construct the log message instead of
using dynamic memory.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.08%) to 77.916% when pulling d9ede32 on grondo:cleanups into 9c78848 on flux-framework:master.

garlick added some commits May 14, 2017

libflux/reparent: drop deprecated class
Overlay rewiring support will be redesigned;
For now, drop the "lspeer" and "reparent" API calls.

Update the one user of these functions, the flux-comms
command.  Drop the "reparent" subcommand, and modify the
"idle" subcommand to make the lspeer RPC directly instead
of relying on the API.
broker: drop cmb.reparent request handler
Drop the cmb.reparent request handler and associated
overlay functions, as this functionality will be
redesigned and currently has no users.
@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 14, 2017

I added a few more commits. In the logging module, I changed the vasnprintf that was the subject of two earlier commits to a vsnprintf on the stack.

I went ahead and removed the reparent module, updated the flux-comms command accordingly, and then dropped the broker code associated with dynamic parent overlay connections. I apologize for not doing this when we got rid of the live module (the main user), since you then had to sort it out in your cleanup.

All done for now. Feel free to squash down when you're happy and I'll press the button.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage increased (+0.04%) to 78.033% when pulling f254964 on grondo:cleanups into 9c78848 on flux-framework:master.

grondo added some commits May 15, 2017

libflux/test: fix leaks in reactor tests
Plug a couple leaks in the reactor test itself to make it possible to
run valgrind on the unit test executable.
libflux/message: fix leak in flux_msg_set_topic
On topic deletion, flux_msg_set_topic() was deleting the topic frame
but not freeing the associated memory with zframe_destroy ().
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 15, 2017

Coverage Status

Coverage increased (+0.05%) to 78.045% when pulling f18d6e1 on grondo:cleanups into 9c78848 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 15, 2017

Thanks for the extra cleanup, looks great!

I added a couple minor leak cleanups, please check those out.

Do you think this needs squashing? I'll check back in after lunch.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 15, 2017

Good catch on the flux_msg_t leak! OK, since we have multiple authors here, maybe the small amount of incremental development is no big deal. Merging.

@garlick garlick merged commit 23facbe into flux-framework:master May 15, 2017

4 checks passed

codecov/patch 81.35% of diff hit (target 77.73%)
Details
codecov/project 77.79% (+0.05%) compared to 9c78848
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 78.045%
Details

@grondo grondo deleted the grondo:cleanups branch May 15, 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.