Hi Owen, Thanks for fixing this test. Some comments below.
On Wed, Jun 16, 2021 at 02:07:24PM -0400, ohily...@iol.unh.edu wrote: > From: Owen Hilyard <ohily...@iol.unh.edu> > > Fixes for a few memory leaks in the cmdline_autotest unit test. > > All of the leaks were related to not freeing the commandline struct > after testing had completed. > > Fixes: dbb860e03e ("cmdline: tests") > > Signed-off-by: Owen Hilyard <ohily...@iol.unh.edu> > Reviewed-by: David Marchand <david.march...@redhat.com> Please use "-v $version" when sending a new version of the patch. You can find examples in doc/guides/contributing/patches.rst > --- > app/test/test_cmdline_lib.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/app/test/test_cmdline_lib.c b/app/test/test_cmdline_lib.c > index bd72df0da..19228c9a5 100644 > --- a/app/test/test_cmdline_lib.c > +++ b/app/test/test_cmdline_lib.c > @@ -71,10 +71,12 @@ test_cmdline_parse_fns(void) > if (cmdline_complete(cl, "buffer", &i, NULL, sizeof(dst)) >= 0) > goto error; > > + cmdline_free(cl); > return 0; > > error: > printf("Error: function accepted null parameter!\n"); > + cmdline_free(cl); > return -1; > } > > @@ -140,32 +142,43 @@ static int > test_cmdline_socket_fns(void) > { > cmdline_parse_ctx_t ctx; > + struct cmdline *cl; > > - if (cmdline_stdin_new(NULL, "prompt") != NULL) > + cl = cmdline_stdin_new(NULL, "prompt"); > + if (cl != NULL) > goto error; > - if (cmdline_stdin_new(&ctx, NULL) != NULL) > + cl = cmdline_stdin_new(&ctx, NULL); > + if (cl != NULL) > goto error; > - if (cmdline_file_new(NULL, "prompt", "/dev/null") != NULL) > + cl = cmdline_file_new(NULL, "prompt", "/dev/null"); > + if (cl != NULL) > goto error; > - if (cmdline_file_new(&ctx, NULL, "/dev/null") != NULL) > + cl = cmdline_file_new(&ctx, NULL, "/dev/null"); > + if (cl != NULL) > goto error; > - if (cmdline_file_new(&ctx, "prompt", NULL) != NULL) > + cl = cmdline_file_new(&ctx, "prompt", NULL); > + if (cl != NULL) > goto error; > - if (cmdline_file_new(&ctx, "prompt", "-/invalid/~/path") != NULL) { > + cl = cmdline_file_new(&ctx, "prompt", "-/invalid/~/path"); > + if (cl != NULL) { > printf("Error: succeeded in opening invalid file for reading!"); > + cmdline_free(cl); > return -1; > } > - if (cmdline_file_new(&ctx, "prompt", "/dev/null") == NULL) { > + cl = cmdline_file_new(&ctx, "prompt", "/dev/null"); > + if (cl == NULL) { > printf("Error: failed to open /dev/null for reading!"); > + cmdline_free(cl); > return -1; > } This last cmdline_free() is not needed, because the test is (cl == NULL) > > /* void functions */ > cmdline_stdin_exit(NULL); > - > + cmdline_free(cl); I would keep an empty line here, to highlight that the comment only refers to cmdline_stdin_exit(). > return 0; > error: > printf("Error: function accepted null parameter!\n"); > + cmdline_free(cl); > return -1; > } > > @@ -198,6 +211,7 @@ test_cmdline_fns(void) > cmdline_interact(NULL); > cmdline_quit(NULL); > In this function test_cmdline_fns(), there is the same scheme than in test_cmdline_socket_fns(), so I think you should apply the same kind of modifications (even if a non-NULL return value should not happen): - if (cmdline_new(NULL, prompt, 0, 0) != NULL) + cl = cmdline_new(NULL, prompt, 0, 0); + if (cl != NULL) Maybe the 2 tested error cases that are expected to return NULL should go before the correct one, so that it's not needed to have another cl pointer variable. > + cmdline_free(cl); > return 0; > > error: > -- > 2.30.2 >