[ndctl] ndctl,daxctl: Ensure allocated contexts are released before exit
diff mbox series

Message ID 20200218155314.89123-1-vaibhav@linux.ibm.com
State New
Headers show
Series
  • [ndctl] ndctl,daxctl: Ensure allocated contexts are released before exit
Related show

Commit Message

Vaibhav Jain Feb. 18, 2020, 3:53 p.m. UTC
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(-)

Comments

Dan Williams Feb. 18, 2020, 4:40 p.m. UTC | #1
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?
Vaibhav Jain Feb. 18, 2020, 7:13 p.m. UTC | #2
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)

Patch
diff mbox series

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__ */