| Message ID | 20200218155314.89123-1-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl] ndctl,daxctl: Ensure allocated contexts are released before exit | expand |
On Tue, Feb 18, 2020 at 7:53 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote: > > Presently main_handle_internal_command() will simply call exit() on > the return value from run_builtin(). This prevents release of allocated > contexts 'struct ndctl_ctx' or 'struct daxctl_ctx' when the main > thread exits. > There is ultimately no leak since process exit cleans up all resources. Does this address a functional problem, or is it just a hygiene fixup?
Dan Williams <dan.j.williams@intel.com> writes: > On Tue, Feb 18, 2020 at 7:53 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote: >> >> Presently main_handle_internal_command() will simply call exit() on >> the return value from run_builtin(). This prevents release of allocated >> contexts 'struct ndctl_ctx' or 'struct daxctl_ctx' when the main >> thread exits. >> > > There is ultimately no leak since process exit cleans up all > resources. Does this address a functional problem, or is it just a > hygiene fixup? I am trying to implement support for a new dimm type in ndctl and was trying to debug a potential memory leak via valgrind/memcheck when came across this issue. Without this patch, memcheck reports lots of leaking reachable memory at ndctl exit that made the task of isolating real leak bit problematic. Below are the run logs of a 'ndctl list' command without and with the patch applied. # Without the patch $ valgrind --leak-check=full ndctl/.libs/ndctl list ==132738== Memcheck, a memory error detector ==132738== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==132738== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info ==132738== Command: ndctl/.libs/ndctl list ==132738== ==132738== ==132738== HEAP SUMMARY: ==132738== in use at exit: 16,564 bytes in 264 blocks ==132738== total heap usage: 920 allocs, 656 frees, 466,916 bytes allocated ==132738== ==132738== LEAK SUMMARY: ==132738== definitely lost: 0 bytes in 0 blocks ==132738== indirectly lost: 0 bytes in 0 blocks ==132738== possibly lost: 0 bytes in 0 blocks ==132738== still reachable: 16,564 bytes in 264 blocks ==132738== suppressed: 0 bytes in 0 blocks ==132738== Reachable blocks (those to which a pointer was found) are not shown. ==132738== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==132738== ==132738== For lists of detected and suppressed errors, rerun with: -s ==132738== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) # With the patch applied $ valgrind --leak-check=full ndctl/.libs/ndctl list ==132759== Memcheck, a memory error detector ==132759== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==132759== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info ==132759== Command: ndctl/.libs/ndctl list ==132759== ==132759== ==132759== HEAP SUMMARY: ==132759== in use at exit: 0 bytes in 0 blocks ==132759== total heap usage: 920 allocs, 920 frees, 466,916 bytes allocated ==132759== ==132759== All heap blocks were freed -- no leaks are possible ==132759== ==132759== For lists of detected and suppressed errors, rerun with: -s ==132759== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c index 1ab073200313..3e2b0c94002b 100644 --- a/daxctl/daxctl.c +++ b/daxctl/daxctl.c @@ -79,7 +79,7 @@ static struct cmd_struct commands[] = { int main(int argc, const char **argv) { struct daxctl_ctx *ctx; - int rc; + int rc, out; /* Look for flags.. */ argv++; @@ -100,10 +100,13 @@ int main(int argc, const char **argv) rc = daxctl_new(&ctx); if (rc) goto out; - main_handle_internal_command(argc, argv, ctx, commands, - ARRAY_SIZE(commands), PROG_DAXCTL); + rc = main_handle_internal_command(argc, argv, ctx, commands, + ARRAY_SIZE(commands), PROG_DAXCTL, &out); daxctl_unref(ctx); - fprintf(stderr, "Unknown command: '%s'\n", argv[0]); + if (!rc) + fprintf(stderr, "Unknown command: '%s'\n", argv[0]); + + return out; out: return 1; } diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c index 6c4975c9f841..6c3a1bb339fc 100644 --- a/ndctl/ndctl.c +++ b/ndctl/ndctl.c @@ -112,7 +112,7 @@ static struct cmd_struct commands[] = { int main(int argc, const char **argv) { struct ndctl_ctx *ctx; - int rc; + int rc, out; /* Look for flags.. */ argv++; @@ -133,10 +133,14 @@ int main(int argc, const char **argv) rc = ndctl_new(&ctx); if (rc) goto out; - main_handle_internal_command(argc, argv, ctx, commands, - ARRAY_SIZE(commands), PROG_NDCTL); + rc = main_handle_internal_command(argc, argv, ctx, commands, + ARRAY_SIZE(commands), PROG_NDCTL, + &out); ndctl_unref(ctx); - fprintf(stderr, "Unknown command: '%s'\n", argv[0]); + if (!rc) + fprintf(stderr, "Unknown command: '%s'\n", argv[0]); + + return out; out: return 1; } diff --git a/util/main.c b/util/main.c index 4f925f84966a..19894d86b914 100644 --- a/util/main.c +++ b/util/main.c @@ -121,11 +121,12 @@ out: return status; } -void main_handle_internal_command(int argc, const char **argv, void *ctx, - struct cmd_struct *cmds, int num_cmds, enum program prog) +int main_handle_internal_command(int argc, const char **argv, void *ctx, + struct cmd_struct *cmds, int num_cmds, enum program prog, + int *out) { const char *cmd = argv[0]; - int i; + int i, handled = 0; /* Turn "<binary> cmd --help" into "<binary> help cmd" */ if (argc > 1 && !strcmp(argv[1], "--help")) { @@ -137,6 +138,9 @@ void main_handle_internal_command(int argc, const char **argv, void *ctx, struct cmd_struct *p = cmds+i; if (strcmp(p->cmd, cmd)) continue; - exit(run_builtin(p, argc, argv, ctx, prog)); + *out = run_builtin(p, argc, argv, ctx, prog); + handled = 1; } + + return handled; } diff --git a/util/main.h b/util/main.h index 35fb33e63049..4d4ea1dc1af7 100644 --- a/util/main.h +++ b/util/main.h @@ -35,8 +35,9 @@ struct cmd_struct { int main_handle_options(const char ***argv, int *argc, const char *usage_msg, struct cmd_struct *cmds, int num_cmds); -void main_handle_internal_command(int argc, const char **argv, void *ctx, - struct cmd_struct *cmds, int num_cmds, enum program prog); +int main_handle_internal_command(int argc, const char **argv, void *ctx, + struct cmd_struct *cmds, int num_cmds, enum program prog, + int *out); int help_show_man_page(const char *cmd, const char *util_name, const char *viewer); #endif /* __MAIN_H__ */
Presently main_handle_internal_command() will simply call exit() on the return value from run_builtin(). This prevents release of allocated contexts 'struct ndctl_ctx' or 'struct daxctl_ctx' when the main thread exits. To fix this behavior so that allocated context are properly deallocated, the patch updates main_handle_internal_command() removing the call to exit() and instead storing the return value from run_builtin() into a new out-argument to the function named 'int *out'. Also main_handle_internal_command() now returns a boolean value indicating if the given command was handled or not. With the above change, daxctl/main() and ndctl/main() are updated to pass this extra argument 'out' to main_handle_internal_command() and handle its return value to possibly indicate an error for "Unknown command" or exiting with the code indicated by 'out'. Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- daxctl/daxctl.c | 11 +++++++---- ndctl/ndctl.c | 12 ++++++++---- util/main.c | 12 ++++++++---- util/main.h | 5 +++-- 4 files changed, 26 insertions(+), 14 deletions(-)