From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 468A5A0C41; Wed, 23 Jun 2021 10:43:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BECA64003F; Wed, 23 Jun 2021 10:43:29 +0200 (CEST) Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by mails.dpdk.org (Postfix) with ESMTP id 506954003E for ; Wed, 23 Jun 2021 10:43:28 +0200 (CEST) Received: by mail-wr1-f41.google.com with SMTP id j2so1641951wrs.12 for ; Wed, 23 Jun 2021 01:43:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=OSD0Op5+ihtmCAxiDZPxQundx8rjrBWRsq+WSOTLQgc=; b=HgI6zDf+DYVRZsDUr4haxI8RIye99FRDIP4APHpVCJXFQOU9BhNgls00SaGIhBBDS/ V5KW5LOoc1OWKiy50HqRaB8HW7DC1BrbsLSfgX9k0+jk5Z1I75G1DuJHF18qy3lXxFS0 w1+p6oQHJ63ZjMLnBuPoqNS6KDMRdjwCGc8elkWVLvpfRabCUm18jqdw1ocDJOB+P/KJ T2CSZNeeBd+wW6ddddT+9BhiIzVACzhCqyBkoR9secCzXYFtHG7uL2ldIcAOUrbn6Q2c kt8vhr0yRYyKOM3Us6U37YxTqSOV5xzkfbNKy1FBs1UPQOctkAcYXAJNx6zCi+l0SqRb 3aOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=OSD0Op5+ihtmCAxiDZPxQundx8rjrBWRsq+WSOTLQgc=; b=OSl6b2JOZk4oEfkIlgbncEoxxR+/C45GDAebGmfV0kunp6T8yrGgkRtzjg84HcFXAv B3+UwhkGyjZSoWyIDENxrDyAvLG4b6VmcN5DemK7q9gUnhqnJeieJaguu/DyxzDkv8Qq wTvDipkEFJYXRSp/FurPpveti0doPVkeeJNE8KbiSCV1sUkNlSZ4Ov1p2gYwG2nXx/kv FmvuB1Etk3RbZwqq7KcZkbhHr0DBoZJR3blOdCMyyYJMbr9HUUjVYdN5GJ4PTpf1Q50w t+m2A5TeUd5eYqTcYQPZLDyDh8gqIbUsZwIG1GCkzuxZ+cwl1CoNW23k2Ng0Y1y0P1wi xx0A== X-Gm-Message-State: AOAM531/0aRK4ke+vY0bUZbp2DC/lFynjeqNcyRFGh2uoFNN9kor4GiL fYbyvON9M0WB8MFGcPXRhbk5Ag== X-Google-Smtp-Source: ABdhPJytSvk7Eu9Cism4c8p305Uu9UH5j4Y/QbuloDhfN6UeilC75csfHLaFbY54c1B2akOiqeHLRg== X-Received: by 2002:adf:fb8c:: with SMTP id a12mr10261748wrr.406.1624437807942; Wed, 23 Jun 2021 01:43:27 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id m8sm2099499wrz.97.2021.06.23.01.43.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Jun 2021 01:43:27 -0700 (PDT) Date: Wed, 23 Jun 2021 10:43:26 +0200 From: Olivier Matz To: ohilyard@iol.unh.edu Cc: dev@dpdk.org, david.marchand@redhat.com, stable@dpdk.org Message-ID: References: <20210616162553.350074-1-ohilyard@iol.unh.edu> <20210616180724.355217-1-ohilyard@iol.unh.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210616180724.355217-1-ohilyard@iol.unh.edu> Subject: Re: [dpdk-dev] [PATCH] tests/cmdline: fix memory leaks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > 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 > Reviewed-by: David Marchand 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 >