* [dpdk-stable] [PATCH] tests/cmdline: fix memory leaks [not found] <20210616162553.350074-1-ohilyard@iol.unh.edu> @ 2021-06-16 18:07 ` ohilyard 2021-06-23 8:43 ` Olivier Matz 2021-06-23 18:06 ` [dpdk-stable] [PATCH v3] " ohilyard 0 siblings, 2 replies; 6+ messages in thread From: ohilyard @ 2021-06-16 18:07 UTC (permalink / raw) To: olivier.matz; +Cc: dev, david.marchand, stable, Owen Hilyard From: Owen Hilyard <ohilyard@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 <ohilyard@iol.unh.edu> Reviewed-by: David Marchand <david.marchand@redhat.com> --- 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; } /* void functions */ cmdline_stdin_exit(NULL); - + cmdline_free(cl); 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); + cmdline_free(cl); return 0; error: -- 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH] tests/cmdline: fix memory leaks 2021-06-16 18:07 ` [dpdk-stable] [PATCH] tests/cmdline: fix memory leaks ohilyard @ 2021-06-23 8:43 ` Olivier Matz 2021-06-23 18:06 ` [dpdk-stable] [PATCH v3] " ohilyard 1 sibling, 0 replies; 6+ messages in thread From: Olivier Matz @ 2021-06-23 8:43 UTC (permalink / raw) To: ohilyard; +Cc: dev, david.marchand, stable Hi Owen, Thanks for fixing this test. Some comments below. On Wed, Jun 16, 2021 at 02:07:24PM -0400, ohilyard@iol.unh.edu wrote: > From: Owen Hilyard <ohilyard@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 <ohilyard@iol.unh.edu> > Reviewed-by: David Marchand <david.marchand@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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-stable] [PATCH v3] tests/cmdline: fix memory leaks 2021-06-16 18:07 ` [dpdk-stable] [PATCH] tests/cmdline: fix memory leaks ohilyard 2021-06-23 8:43 ` Olivier Matz @ 2021-06-23 18:06 ` ohilyard 2021-06-24 12:07 ` Olivier Matz 2021-06-24 13:25 ` David Marchand 1 sibling, 2 replies; 6+ messages in thread From: ohilyard @ 2021-06-23 18:06 UTC (permalink / raw) To: olivier.matz; +Cc: dev, stable, david.marchand, Owen Hilyard From: Owen Hilyard <ohilyard@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 <ohilyard@iol.unh.edu> Reviewed-by: David Marchand <david.marchand@redhat.com> --- app/test/test_cmdline_lib.c | 40 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/app/test/test_cmdline_lib.c b/app/test/test_cmdline_lib.c index bd72df0da..b476b2594 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,44 @@ 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; } /* void functions */ cmdline_stdin_exit(NULL); + cmdline_free(cl); return 0; error: printf("Error: function accepted null parameter!\n"); + cmdline_free(cl); return -1; } @@ -176,13 +190,14 @@ test_cmdline_fns(void) struct cmdline *cl; memset(&ctx, 0, sizeof(ctx)); - cl = cmdline_new(&ctx, "test", -1, -1); - if (cl == NULL) + cl = cmdline_new(NULL, "prompt", 0, 0); + if (cl != NULL) goto error; - - if (cmdline_new(NULL, "prompt", 0, 0) != NULL) + cl = cmdline_new(&ctx, NULL, 0, 0); + if (cl != NULL) goto error; - if (cmdline_new(&ctx, NULL, 0, 0) != NULL) + cl = cmdline_new(&ctx, "test", -1, -1); + if (cl == NULL) goto error; if (cmdline_in(NULL, "buffer", CMDLINE_TEST_BUFSIZE) >= 0) goto error; @@ -198,6 +213,7 @@ test_cmdline_fns(void) cmdline_interact(NULL); cmdline_quit(NULL); + cmdline_free(cl); return 0; error: -- 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH v3] tests/cmdline: fix memory leaks 2021-06-23 18:06 ` [dpdk-stable] [PATCH v3] " ohilyard @ 2021-06-24 12:07 ` Olivier Matz 2021-06-24 13:24 ` David Marchand 2021-06-24 13:25 ` David Marchand 1 sibling, 1 reply; 6+ messages in thread From: Olivier Matz @ 2021-06-24 12:07 UTC (permalink / raw) To: ohilyard; +Cc: dev, stable, david.marchand Hi Owen, One small issue remain, please see below. On Wed, Jun 23, 2021 at 02:06:45PM -0400, ohilyard@iol.unh.edu wrote: > From: Owen Hilyard <ohilyard@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 <ohilyard@iol.unh.edu> > Reviewed-by: David Marchand <david.marchand@redhat.com> > --- > app/test/test_cmdline_lib.c | 40 ++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/app/test/test_cmdline_lib.c b/app/test/test_cmdline_lib.c > index bd72df0da..b476b2594 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,44 @@ 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; > } The cmdline_free(cl) after a if (cl == NULL) is not needed. After that change, you can add my ack in v4. Thanks, Olivier > > /* void functions */ > cmdline_stdin_exit(NULL); > > + cmdline_free(cl); > return 0; > error: > printf("Error: function accepted null parameter!\n"); > + cmdline_free(cl); > return -1; > } > > @@ -176,13 +190,14 @@ test_cmdline_fns(void) > struct cmdline *cl; > > memset(&ctx, 0, sizeof(ctx)); > - cl = cmdline_new(&ctx, "test", -1, -1); > - if (cl == NULL) > + cl = cmdline_new(NULL, "prompt", 0, 0); > + if (cl != NULL) > goto error; > - > - if (cmdline_new(NULL, "prompt", 0, 0) != NULL) > + cl = cmdline_new(&ctx, NULL, 0, 0); > + if (cl != NULL) > goto error; > - if (cmdline_new(&ctx, NULL, 0, 0) != NULL) > + cl = cmdline_new(&ctx, "test", -1, -1); > + if (cl == NULL) > goto error; > if (cmdline_in(NULL, "buffer", CMDLINE_TEST_BUFSIZE) >= 0) > goto error; > @@ -198,6 +213,7 @@ test_cmdline_fns(void) > cmdline_interact(NULL); > cmdline_quit(NULL); > > + cmdline_free(cl); > return 0; > > error: > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH v3] tests/cmdline: fix memory leaks 2021-06-24 12:07 ` Olivier Matz @ 2021-06-24 13:24 ` David Marchand 0 siblings, 0 replies; 6+ messages in thread From: David Marchand @ 2021-06-24 13:24 UTC (permalink / raw) To: Olivier Matz; +Cc: Owen Hilyard, dev, dpdk stable On Thu, Jun 24, 2021 at 2:08 PM Olivier Matz <olivier.matz@6wind.com> wrote: > > - 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; > > } > > The cmdline_free(cl) after a if (cl == NULL) is not needed. I took the liberty of doing the change when applying and added your ack. Thanks. -- David Marchand ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH v3] tests/cmdline: fix memory leaks 2021-06-23 18:06 ` [dpdk-stable] [PATCH v3] " ohilyard 2021-06-24 12:07 ` Olivier Matz @ 2021-06-24 13:25 ` David Marchand 1 sibling, 0 replies; 6+ messages in thread From: David Marchand @ 2021-06-24 13:25 UTC (permalink / raw) To: Owen Hilyard; +Cc: Olivier Matz, dev, dpdk stable On Wed, Jun 23, 2021 at 8:06 PM <ohilyard@iol.unh.edu> wrote: > > From: Owen Hilyard <ohilyard@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") Updated sha1. Cc: stable@dpdk.org > > Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu> > Reviewed-by: David Marchand <david.marchand@redhat.com> Acked-by: Olivier Matz <olivier.matz@6wind.com> Applied, thanks. -- David Marchand ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-24 13:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210616162553.350074-1-ohilyard@iol.unh.edu>
2021-06-16 18:07 ` [dpdk-stable] [PATCH] tests/cmdline: fix memory leaks ohilyard
2021-06-23 8:43 ` Olivier Matz
2021-06-23 18:06 ` [dpdk-stable] [PATCH v3] " ohilyard
2021-06-24 12:07 ` Olivier Matz
2021-06-24 13:24 ` David Marchand
2021-06-24 13:25 ` David Marchand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).