From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f54.google.com (mail-wg0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 79F9768CD for ; Tue, 28 Jan 2014 17:05:56 +0100 (CET) Received: by mail-wg0-f54.google.com with SMTP id x13so1150271wgg.21 for ; Tue, 28 Jan 2014 08:07:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=wufpMTow2a+uL3KJXZqbrnJSakQiB7rWyXwxUo/XbTI=; b=cF88r0JsK74DqXKEXZPDo6pP4KKzH1rNPlB8VBgd66GHpbHJcQFS9UPylRMxWpFDae SHGlFueDWU8FlSgvE74gbfpYJ8tu/cmP0B9RuKSlSLOb3G0fD8dqiFeEuJ5j/cnsneKx NX8tnTorzYyaqZKd/975P4nIh2JyAUVJJ6pxBPr0fA0TbLrtAZ7FiK1gIlD4o4L/ao/A Kp8Z+yYR41u9uarAGvbFGZceeeJL+yDV135N9u8DjFOTh6K5T4KdcKf0Tb9x9evfg3Ge nKCxIN/fwZ8RoU3Vvabf+lF3dYYJftgU5qi8eD3eAMXeikVid1NUtt2Yap6POkj1vCsX +onA== X-Gm-Message-State: ALoCoQkC11dvLZsSeE+oWhhhT6J8+yrD4Xwb5qtPJytyjP41TGkLBHiEdHfPdM6iycH9Hh71jLRs X-Received: by 10.194.142.231 with SMTP id rz7mr74021wjb.89.1390925234897; Tue, 28 Jan 2014 08:07:14 -0800 (PST) Received: from glumotte.dev.6wind.com (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id d6sm36407821wic.9.2014.01.28.08.07.13 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Jan 2014 08:07:14 -0800 (PST) From: Olivier Matz To: dev@dpdk.org Date: Tue, 28 Jan 2014 17:06:38 +0100 Message-Id: <1390925204-10800-6-git-send-email-olivier.matz@6wind.com> X-Mailer: git-send-email 1.8.4.rc3 In-Reply-To: <1390925204-10800-1-git-send-email-olivier.matz@6wind.com> References: <1390925204-10800-1-git-send-email-olivier.matz@6wind.com> Subject: [dpdk-dev] [PATCH 05/11] kvargs: rework API to fix memory leak X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Jan 2014 16:05:57 -0000 Before the patch, a call to rte_kvargs_tokenize() resulted in a call to strdup() to allocate a modifiable copy of the argument string. This string was never freed, excepted in the error cases of rte_kvargs_tokenize() where rte_free() was wrongly called instead of free(). In other cases, freeing this string was impossible as the pointer not saved. This patch introduces rte_kvargs_free() in order to free the structure properly. The pointer to the duplicated string is now kept in the rte_kvargs structure. A call to rte_kvargs_parse() directly allocates the structure, making rte_kvargs_init() useless. The only drawback of this API change is that a key/value associations cannot be added to an existing kvlist. But it's not used today, and there is not obvious use case for that. Signed-off-by: Olivier Matz --- lib/librte_kvargs/Makefile | 3 +- lib/librte_kvargs/rte_kvargs.c | 64 ++++++++++++++++++-------------------- lib/librte_kvargs/rte_kvargs.h | 42 +++++++++++-------------- lib/librte_pmd_pcap/rte_eth_pcap.c | 27 ++++++++-------- 4 files changed, 63 insertions(+), 73 deletions(-) diff --git a/lib/librte_kvargs/Makefile b/lib/librte_kvargs/Makefile index c4c2855..69fd414 100644 --- a/lib/librte_kvargs/Makefile +++ b/lib/librte_kvargs/Makefile @@ -43,8 +43,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_KVARGS) := rte_kvargs.c INCS := rte_kvargs.h SYMLINK-$(CONFIG_RTE_LIBRTE_KVARGS)-include := $(INCS) -# this lib needs eal and malloc +# this lib needs eal DEPDIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += lib/librte_eal -DEPDIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += lib/librte_malloc include $(RTE_SDK)/mk/rte.lib.mk diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c index 0bf46fe..9c52ce4 100644 --- a/lib/librte_kvargs/rte_kvargs.c +++ b/lib/librte_kvargs/rte_kvargs.c @@ -34,25 +34,14 @@ #include #include #include +#include -#include #include #include #include "rte_kvargs.h" /* - * Initialize a rte_kvargs structure to an empty key/value list. - */ -int -rte_kvargs_init(struct rte_kvargs *kvlist) -{ - kvlist->count = 0; - memset(kvlist->pairs, 0, sizeof(kvlist->pairs)); - return 0; -} - -/* * Add a key-value pair at the end of a given key/value list. * Return an error if the list is full or if the key is duplicated. */ @@ -93,7 +82,6 @@ static int rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params) { unsigned i, count; - char *args; char *pairs[RTE_KVARGS_MAX]; char *pair[2]; @@ -106,13 +94,13 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params) /* Copy the const char *params to a modifiable string * to pass to rte_strsplit */ - args = strdup(params); - if(args == NULL){ + kvlist->str = strdup(params); + if (kvlist->str == NULL) { RTE_LOG(ERR, PMD, "Cannot parse arguments: not enough memory\n"); return -1; } - count = rte_strsplit(args, strnlen(args, MAX_ARG_STRLEN), pairs, + count = rte_strsplit(kvlist->str, strnlen(kvlist->str, MAX_ARG_STRLEN), pairs, RTE_KVARGS_MAX, RTE_KVARGS_PAIRS_DELIM); for (i = 0; i < count; i++) { @@ -127,17 +115,13 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params) RTE_LOG(ERR, PMD, "Cannot parse arguments: wrong key or value\n" "params=<%s>\n", params); - goto error; + return -1; } if (rte_kvargs_add_pair(kvlist, pair[0], pair[1]) < 0) - goto error; + return -1; } return 0; - -error: - rte_free(args); - return -1; } /* @@ -222,25 +206,39 @@ rte_kvargs_process(const struct rte_kvargs *kvlist, return 0; } +/* free the rte_kvargs structure */ +void +rte_kvargs_free(struct rte_kvargs *kvlist) +{ + if (kvlist->str != NULL) + free(kvlist->str); + free(kvlist); +} + /* * Parse the arguments "key=value;key=value;..." string and return * an allocated structure that contains a key/value list. Also * check if only valid keys were used. */ -int -rte_kvargs_parse(struct rte_kvargs *kvlist, - const char *args, - const char *valid_keys[]) +struct rte_kvargs * +rte_kvargs_parse(const char *args, const char *valid_keys[]) { + struct rte_kvargs *kvlist; - int ret; + kvlist = malloc(sizeof(*kvlist)); + if (kvlist == NULL) + return NULL; + memset(kvlist, 0, sizeof(*kvlist)); - ret = rte_kvargs_tokenize(kvlist, args); - if (ret < 0) - return ret; + if (rte_kvargs_tokenize(kvlist, args) < 0) { + rte_kvargs_free(kvlist); + return NULL; + } - if (valid_keys == NULL) - return 0; + if (valid_keys != NULL && check_for_valid_keys(kvlist, valid_keys) < 0) { + rte_kvargs_free(kvlist); + return NULL; + } - return check_for_valid_keys(kvlist, valid_keys); + return kvlist; } diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h index 577f00b..47d78af 100644 --- a/lib/librte_kvargs/rte_kvargs.h +++ b/lib/librte_kvargs/rte_kvargs.h @@ -70,47 +70,41 @@ struct rte_kvargs_pair { /** Store a list of key/value associations */ struct rte_kvargs { + char *str; /**< copy of the argument string */ unsigned count; /**< number of entries in the list */ struct rte_kvargs_pair pairs[RTE_KVARGS_MAX]; /**< list of key/values */ }; /** - * Initialize an empty rte_kvargs structure + * Allocate a rte_kvargs and store key/value associations from a string * - * Set the content of the rte_kvargs structure given as a parameter - * to an empty list of key/value. + * The function allocates and fills a rte_kvargs structure from a given + * string whose format is key1=value1;key2=value2;... * - * @param kvlist - * The rte_kvargs structure + * The structure can be freed with rte_kvargs_free(). + * + * @param args + * The input string containing the key/value associations + * @param valid_keys + * A list of valid keys (table of const char *, the last must be NULL). + * This argument is ignored if NULL * * @return - * - 0 on success - * - Negative on error + * - A pointer to an allocated rte_kvargs structure on success + * - NULL on error */ -int rte_kvargs_init(struct rte_kvargs *kvlist); +struct rte_kvargs *rte_kvargs_parse(const char *args, const char *valid_keys[]); /** - * Append key/value associations in a rte_kvargs structure from a string + * Free a rte_kvargs structure * - * The function fills a rte_kvargs structure from a string whose format - * is key1=value1;key2=value2;... - * The key/value associations are appended to those which are already - * present in the rte_kvargs structure. + * Free a rte_kvargs structure previously allocated with + * rte_kvargs_parse(). * * @param kvlist * The rte_kvargs structure - * @param args - * The input string containing the key/value associations - * @param valid_keys - * A list of valid keys (table of const char *, the last must be NULL). - * This argument is ignored if NULL - * - * @return - * - 0 on success - * - Negative on error */ -int rte_kvargs_parse(struct rte_kvargs *kvlist, - const char *args, const char *valid_keys[]); +void rte_kvargs_free(struct rte_kvargs *kvlist); /** * Call a handler function for each key/value matching the key diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c index 2006b35..1f46cc9 100644 --- a/lib/librte_pmd_pcap/rte_eth_pcap.c +++ b/lib/librte_pmd_pcap/rte_eth_pcap.c @@ -702,30 +702,29 @@ rte_pmd_pcap_init(const char *name, const char *params) { unsigned numa_node, using_dumpers = 0; int ret; - struct rte_kvargs kvlist; + struct rte_kvargs *kvlist; struct rx_pcaps pcaps; struct tx_pcaps dumpers; RTE_LOG(INFO, PMD, "Initializing pmd_pcap for %s\n", name); - rte_kvargs_init(&kvlist); - numa_node = rte_socket_id(); gettimeofday(&start_time, NULL); start_cycles = rte_get_timer_cycles(); hz = rte_get_timer_hz(); - if (rte_kvargs_parse(&kvlist, params, valid_arguments) < 0) + kvlist = rte_kvargs_parse(params, valid_arguments); + if (kvlist == NULL) return -1; /* * If iface argument is passed we open the NICs and use them for * reading / writing */ - if (rte_kvargs_count(&kvlist, ETH_PCAP_IFACE_ARG) == 1) { + if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) { - ret = rte_kvargs_process(&kvlist, ETH_PCAP_IFACE_ARG, + ret = rte_kvargs_process(kvlist, ETH_PCAP_IFACE_ARG, &open_rx_tx_iface, &pcaps.pcaps[0]); if (ret < 0) return -1; @@ -737,13 +736,13 @@ rte_pmd_pcap_init(const char *name, const char *params) * We check whether we want to open a RX stream from a real NIC or a * pcap file */ - if ((pcaps.num_of_rx = rte_kvargs_count(&kvlist, ETH_PCAP_RX_PCAP_ARG))) { - ret = rte_kvargs_process(&kvlist, ETH_PCAP_RX_PCAP_ARG, + if ((pcaps.num_of_rx = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG))) { + ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG, &open_rx_pcap, &pcaps); } else { - pcaps.num_of_rx = rte_kvargs_count(&kvlist, + pcaps.num_of_rx = rte_kvargs_count(kvlist, ETH_PCAP_RX_IFACE_ARG); - ret = rte_kvargs_process(&kvlist, ETH_PCAP_RX_IFACE_ARG, + ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_IFACE_ARG, &open_rx_iface, &pcaps); } @@ -754,15 +753,15 @@ rte_pmd_pcap_init(const char *name, const char *params) * We check whether we want to open a TX stream to a real NIC or a * pcap file */ - if ((dumpers.num_of_tx = rte_kvargs_count(&kvlist, + if ((dumpers.num_of_tx = rte_kvargs_count(kvlist, ETH_PCAP_TX_PCAP_ARG))) { - ret = rte_kvargs_process(&kvlist, ETH_PCAP_TX_PCAP_ARG, + ret = rte_kvargs_process(kvlist, ETH_PCAP_TX_PCAP_ARG, &open_tx_pcap, &dumpers); using_dumpers = 1; } else { - dumpers.num_of_tx = rte_kvargs_count(&kvlist, + dumpers.num_of_tx = rte_kvargs_count(kvlist, ETH_PCAP_TX_IFACE_ARG); - ret = rte_kvargs_process(&kvlist, ETH_PCAP_TX_IFACE_ARG, + ret = rte_kvargs_process(kvlist, ETH_PCAP_TX_IFACE_ARG, &open_tx_iface, &dumpers); } -- 1.8.4.rc3