[dpdk-dev] [PATCH 0/5] Fix issues reported by static analysis tool

[dpdk-dev] [PATCH 0/5] Fix issues reported by static analysis tool

[Pawel Wodkowski]

Klockwork report some issues against current DPDK version. Most of them need
only cosmetic code changes (changing type of variable or adding explicit cast).

One issue related with ring pmd fix real memory leak problem.


Pawel Wodkowski (5):
  rte_timer: fix invalid declaration of rte_timer_cb_t
  librte_kvargs: make rte_kvargs_free() be consistent with other
    "free()"     functions
  pmd ring: fix possible memory leak during devinit
  cmdline: make parse_set_list() use size_t instead of int for low/high 
       parameter
  Fix usage of fgets in various places

 lib/librte_cfgfile/rte_cfgfile.c                |  2 +-
 lib/librte_cmdline/cmdline_parse_portlist.c     |  4 ++--
 lib/librte_eal/bsdapp/eal/eal.c                 |  2 +-
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c |  4 ++--
 lib/librte_eal/linuxapp/eal/eal_memory.c        |  2 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  2 +-
 lib/librte_eal/linuxapp/eal/eal_timer.c         |  2 +-
 lib/librte_kvargs/rte_kvargs.c                  |  4 ++++
 lib/librte_kvargs/rte_kvargs.h                  |  3 ++-
 lib/librte_pmd_ring/rte_eth_ring.c              |  6 +++---
 lib/librte_pmd_virtio/virtio_ethdev.c           |  2 +-
 lib/librte_power/rte_power_acpi_cpufreq.c       | 10 +++++-----
 lib/librte_timer/rte_timer.h                    |  4 ++--
 13 files changed, 26 insertions(+), 21 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of rte_timer_cb_t

[Pawel Wodkowski]

Declaration for function pointer should be
typedef ret_type (*type_name)(args...)
not
typedef ret_type (type_name)(args...)

although compiler treat both of them the same, the static analysis tool
like klocwork complain about that.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_timer/rte_timer.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 4907cf5..327fe4b 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -115,7 +115,7 @@ struct rte_timer;
 /**
  * Callback function type for timer expiry.
  */
-typedef void (rte_timer_cb_t)(struct rte_timer *, void *);
+typedef void (*rte_timer_cb_t)(struct rte_timer *, void *);
 
 #define MAX_SKIPLIST_DEPTH 10
 
@@ -128,7 +128,7 @@ struct rte_timer
 	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
 	volatile union rte_timer_status status; /**< Status of timer. */
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
-	rte_timer_cb_t *f;     /**< Callback function. */
+	rte_timer_cb_t f;      /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
 };
 
-- 
1.9.1

[dpdk-dev] [PATCH 2/5] librte_kvargs: make rte_kvargs_free() be consistent with other "free()" functions

[Pawel Wodkowski]

It is desired that all type of *_free() functions mimic behaviour of
libc free() function. This function does nothing if given parameter is
NULL. This patch add this behaviour for rte_kvargs_free().

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_kvargs/rte_kvargs.c | 4 ++++
 lib/librte_kvargs/rte_kvargs.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index 8bc1e46..c2dd051 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -174,8 +174,12 @@ rte_kvargs_process(const struct rte_kvargs *kvlist,
 void
 rte_kvargs_free(struct rte_kvargs *kvlist)
 {
+	if (!kvlist)
+		return;
+
 	if (kvlist->str != NULL)
 		free(kvlist->str);
+
 	free(kvlist);
 }
 
diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h
index ef4efab..ae9ae79 100644
--- a/lib/librte_kvargs/rte_kvargs.h
+++ b/lib/librte_kvargs/rte_kvargs.h
@@ -115,7 +115,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist);
  *
  * For each key/value association that matches the given key, calls the
  * handler function with the for a given arg_name passing the value on the
- * dictionary for that key and a given extra argument.
+ * dictionary for that key and a given extra argument. If *kvlist* is NULL
+ * function does nothing.
  *
  * @param kvlist
  *   The rte_kvargs structure
-- 
1.9.1

[dpdk-dev] [PATCH 3/5] pmd ring: fix possible memory leak during devinit

[Pawel Wodkowski]

Free kvlist on function exit to avoid memory leak.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_pmd_ring/rte_eth_ring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_pmd_ring/rte_eth_ring.c b/lib/librte_pmd_ring/rte_eth_ring.c
index a23e933..582a621 100644
--- a/lib/librte_pmd_ring/rte_eth_ring.c
+++ b/lib/librte_pmd_ring/rte_eth_ring.c
@@ -527,7 +527,7 @@ out:
 static int
 rte_pmd_ring_devinit(const char *name, const char *params)
 {
-	struct rte_kvargs *kvlist;
+	struct rte_kvargs *kvlist = NULL;
 	int ret = 0;
 	struct node_action_list *info = NULL;
 
@@ -548,7 +548,7 @@ rte_pmd_ring_devinit(const char *name, const char *params)
 			info = rte_zmalloc("struct node_action_list", sizeof(struct node_action_list) +
 					   (sizeof(struct node_action_pair) * ret), 0);
 			if (!info)
-				goto out;
+				goto out_free;
 
 			info->total = ret;
 			info->list = (struct node_action_pair*)(info + 1);
@@ -567,8 +567,8 @@ rte_pmd_ring_devinit(const char *name, const char *params)
 	}
 
 out_free:
+	rte_kvargs_free(kvlist);
 	rte_free(info);
-out:
 	return ret;
 }
 
-- 
1.9.1

[dpdk-dev] [PATCH 4/5] cmdline: make parse_set_list() use size_t instead of int for low/high parameter

[Pawel Wodkowski]

Fix warning reported by klocwork about size_t to int cast when passing
parameters to parse_set_list().

This patch fix code formating errors that give checkpatch.pl errors
after generating patch.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_cmdline/cmdline_parse_portlist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
index fc6c14e..9c1fe3e 100644
--- a/lib/librte_cmdline/cmdline_parse_portlist.c
+++ b/lib/librte_cmdline/cmdline_parse_portlist.c
@@ -78,7 +78,7 @@ struct cmdline_token_ops cmdline_token_portlist_ops = {
 };
 
 static void
-parse_set_list(cmdline_portlist_t * pl, int low, int high)
+parse_set_list(cmdline_portlist_t *pl, size_t low, size_t high)
 {
 	do {
 		pl->map |= (1 << low++);
@@ -86,7 +86,7 @@ parse_set_list(cmdline_portlist_t * pl, int low, int high)
 }
 
 static int
-parse_ports(cmdline_portlist_t * pl, const char * str)
+parse_ports(cmdline_portlist_t *pl, const char *str)
 {
 	size_t ps, pe;
 	const char *first, *last;
-- 
1.9.1

[dpdk-dev] [PATCH 5/5] Fix usage of fgets in various places

[Pawel Wodkowski]

Declaration of fgets() is
char *fgets(char *str, int size, FILE *stream);

Klocwork complain about passing "sizeof()" as size parameter since
implicit casting size_t to int might cause loss of precision.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c                |  2 +-
 lib/librte_eal/bsdapp/eal/eal.c                 |  2 +-
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c |  4 ++--
 lib/librte_eal/linuxapp/eal/eal_memory.c        |  2 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  2 +-
 lib/librte_eal/linuxapp/eal/eal_timer.c         |  2 +-
 lib/librte_pmd_virtio/virtio_ethdev.c           |  2 +-
 lib/librte_power/rte_power_acpi_cpufreq.c       | 10 +++++-----
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index b81c273..15ef447 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -107,7 +107,7 @@ rte_cfgfile_load(const char *filename, int flags)
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
-	while (fgets(buffer, sizeof(buffer), f) != NULL) {
+	while (fgets(buffer, (int)sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 69f3c03..ca51868 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -134,7 +134,7 @@ eal_parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
 			__func__, filename);
 		fclose(f);
diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index 590cb56..551472c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -115,7 +115,7 @@ get_default_hp_size(void)
 	FILE *fd = fopen(proc_meminfo, "r");
 	if (fd == NULL)
 		rte_panic("Cannot open %s\n", proc_meminfo);
-	while(fgets(buffer, sizeof(buffer), fd)){
+	while (fgets(buffer, (int)sizeof(buffer), fd)) {
 		if (strncmp(buffer, str_hugepagesz, hugepagesz_len) == 0){
 			size = rte_str_to_size(&buffer[hugepagesz_len]);
 			break;
@@ -155,7 +155,7 @@ get_hugepage_dir(uint64_t hugepage_sz)
 	if (default_size == 0)
 		default_size = get_default_hp_size();
 
-	while (fgets(buf, sizeof(buf), fd)){
+	while (fgets(buf, (int)sizeof(buf), fd)) {
 		if (rte_strsplit(buf, sizeof(buf), splitstr, _FIELDNAME_MAX,
 				split_tok) != _FIELDNAME_MAX) {
 			RTE_LOG(ERR, EAL, "Error parsing %s\n", proc_mounts);
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index a67a1b0..0c7f8ce 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -614,7 +614,7 @@ find_numasocket(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi)
 			"%s/%s", hpi->hugedir, internal_config.hugefile_prefix);
 
 	/* parse numa map */
-	while (fgets(buf, sizeof(buf), f) != NULL) {
+	while (fgets(buf, (int)sizeof(buf), f) != NULL) {
 
 		/* ignore non huge page */
 		if (strstr(buf, " huge ") == NULL &&
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 63bcbce..ee4e1d8 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -162,7 +162,7 @@ pci_parse_sysfs_resource(const char *filename, struct rte_pci_device *dev)
 
 	for (i = 0; i<PCI_MAX_RESOURCE; i++) {
 
-		if (fgets(buf, sizeof(buf), f) == NULL) {
+		if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 			RTE_LOG(ERR, EAL,
 				"%s(): cannot read resource\n", __func__);
 			goto error;
diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index ca57916..d5419b9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -253,7 +253,7 @@ check_tsc_flags(void)
 		return;
 	}
 
-	while (fgets(line, sizeof line, stream)) {
+	while (fgets(line, (int)sizeof(line), stream)) {
 		char *constant_tsc;
 		char *nonstop_tsc;
 
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index 9eb0217..0cd0665 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -832,7 +832,7 @@ parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 		PMD_INIT_LOG(ERR, "%s(): cannot read sysfs value %s",
 			     __func__, filename);
 		fclose(f);
diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index a56c9b5..a5b46bb 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -164,7 +164,7 @@ power_set_governor_userspace(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Check if current governor is userspace */
@@ -214,7 +214,7 @@ power_get_available_freqs(struct rte_power_info *pi)
 	f = fopen(fullpath, "r");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Strip the line break if there is */
@@ -223,7 +223,7 @@ power_get_available_freqs(struct rte_power_info *pi)
 		*p = 0;
 
 	/* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
-	count = rte_strsplit(buf, sizeof(buf), freqs,
+	count = rte_strsplit(buf, (int)sizeof(buf), freqs,
 			RTE_MAX_LCORE_FREQS, ' ');
 	if (count <= 0) {
 		RTE_LOG(ERR, POWER, "No available frequency in "
@@ -270,7 +270,7 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, -1);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
@@ -367,7 +367,7 @@ power_set_governor_original(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Check if the governor to be set is the same as current */
-- 
1.9.1

Re: [dpdk-dev] [PATCH 5/5] Fix usage of fgets in various places

[Stephen Hemminger]

On Mon, 23 Feb 2015 15:10:00 +0100
Pawel Wodkowski <pawelx.wodkowski@intel.com> wrote:

> Declaration of fgets() is
> char *fgets(char *str, int size, FILE *stream);
> 
> Klocwork complain about passing "sizeof()" as size parameter since
> implicit casting size_t to int might cause loss of precision.
> 
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>

NAK this is shooting at Unicorns.
The tool is the problem not the code.

Re: [dpdk-dev] [PATCH 5/5] Fix usage of fgets in various places

[Panu Matilainen]

On 02/23/2015 06:00 PM, Stephen Hemminger wrote:
> On Mon, 23 Feb 2015 15:10:00 +0100
> Pawel Wodkowski <pawelx.wodkowski@intel.com> wrote:
>
>> Declaration of fgets() is
>> char *fgets(char *str, int size, FILE *stream);
>>
>> Klocwork complain about passing "sizeof()" as size parameter since
>> implicit casting size_t to int might cause loss of precision.
>>
>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
>
> NAK this is shooting at Unicorns.
> The tool is the problem not the code.

The tool is technically correct, even if loss of precision might be 
unlikely to occur in this context.

However the patch is incorrect, the problem doesn't go away by adding an 
explict cast even if it shuts up the tool.

	- Panu -

Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of rte_timer_cb_t

[Olivier MATZ]

Hi Pawel,

On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> Declaration for function pointer should be
> typedef ret_type (*type_name)(args...)
> not
> typedef ret_type (type_name)(args...)
>
> although compiler treat both of them the same, the static analysis tool
> like klocwork complain about that.

Can you give some details about the reason why klocwork is
complaining?

Looking at the C11 standard, it seems that this syntax is
legal. Please see EXAMPLE 4, page 156 of:
http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf

Regards,
Olivier

Re: [dpdk-dev] [PATCH 4/5] cmdline: make parse_set_list() use size_t instead of int for low/high parameter

[Olivier MATZ]

Hi Pawel,

On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> Fix warning reported by klocwork about size_t to int cast when passing
> parameters to parse_set_list().
>
> This patch fix code formating errors that give checkpatch.pl errors
> after generating patch.
>
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 3/5] pmd ring: fix possible memory leak during devinit

[Olivier MATZ]

On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> Free kvlist on function exit to avoid memory leak.
>
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of rte_timer_cb_t

[Wodkowski, PawelX]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 24, 2015 11:39 AM
> To: Wodkowski, PawelX; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of
> rte_timer_cb_t
> 
> Hi Pawel,
> 
> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> > Declaration for function pointer should be
> > typedef ret_type (*type_name)(args...)
> > not
> > typedef ret_type (type_name)(args...)
> >
> > although compiler treat both of them the same, the static analysis tool
> > like klocwork complain about that.
> 
> Can you give some details about the reason why klocwork is
> complaining?
> 
> Looking at the C11 standard, it seems that this syntax is
> legal. Please see EXAMPLE 4, page 156 of:
> http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf
> 

Legal, might be. Problem is in using it. In struct rte_timer field 'f' if
declared as pointer to rte_timer_cb_t but  __rte_timer_reset() expects
rte_timer_cb_t. This have a little impact to real code but it is inconsistent
with declaration, definition and rest of the library where first syntax is used.
There are some places where second declaration is used but its usage there
is consistent with declaration.

I looked at the code in rest of library and for consistency I changed
typedef rather than function declaration.

Pawel

Re: [dpdk-dev] [PATCH 3/5] pmd ring: fix possible memory leak during devinit

[Pawel Wodkowski]

On 2015-02-24 12:05, Olivier MATZ wrote:
> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
>> Free kvlist on function exit to avoid memory leak.
>>
>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>

Please have in mind that this patch depend on patch 2/5

-- 
Pawel

Re: [dpdk-dev] [PATCH 3/5] pmd ring: fix possible memory leak during devinit

[Olivier MATZ]

On 02/24/2015 12:15 PM, Pawel Wodkowski wrote:
> On 2015-02-24 12:05, Olivier MATZ wrote:
>> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
>>> Free kvlist on function exit to avoid memory leak.
>>>
>>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
>>
>> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>>
>
> Please have in mind that this patch depend on patch 2/5
>

Indeed. I forgot to send the mail to ack the 2/5, sorry.
That's done now.

Thanks,
Olivier

Re: [dpdk-dev] [PATCH 2/5] librte_kvargs: make rte_kvargs_free() be consistent with other "free()" functions

[Olivier MATZ]

On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> It is desired that all type of *_free() functions mimic behaviour of
> libc free() function. This function does nothing if given parameter is
> NULL. This patch add this behaviour for rte_kvargs_free().
>
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of rte_timer_cb_t

[Olivier MATZ]

Hi Pawel,

On 02/24/2015 12:12 PM, Wodkowski, PawelX wrote:
>
>
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, February 24, 2015 11:39 AM
>> To: Wodkowski, PawelX; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of
>> rte_timer_cb_t
>>
>> Hi Pawel,
>>
>> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
>>> Declaration for function pointer should be
>>> typedef ret_type (*type_name)(args...)
>>> not
>>> typedef ret_type (type_name)(args...)
>>>
>>> although compiler treat both of them the same, the static analysis tool
>>> like klocwork complain about that.
>>
>> Can you give some details about the reason why klocwork is
>> complaining?
>>
>> Looking at the C11 standard, it seems that this syntax is
>> legal. Please see EXAMPLE 4, page 156 of:
>> http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf
>>
>
> Legal, might be. Problem is in using it. In struct rte_timer field 'f' if
> declared as pointer to rte_timer_cb_t but  __rte_timer_reset() expects
> rte_timer_cb_t. This have a little impact to real code but it is inconsistent
> with declaration, definition and rest of the library where first syntax is used.
> There are some places where second declaration is used but its usage there
> is consistent with declaration.
>
> I looked at the code in rest of library and for consistency I changed
> typedef rather than function declaration.

OK, got it.
The problem was not the declaration itself but the inconsistency
between the function arguments and the rte_timer structure.

If you plan to do a v2 (maybe for patch 5/5), do you think you can
reword the commit log and title to clarify this a bit more?

Thanks,
Olivier

Re: [dpdk-dev] [PATCH 5/5] Fix usage of fgets in various places

[Stephen Hemminger]

On Tue, 24 Feb 2015 11:20:33 +0200
Panu Matilainen <pmatilai@redhat.com> wrote:

> The tool is technically correct, even if loss of precision might be 
> unlikely to occur in this context

Overflow is not there in the code.
That is why I said "shooting Unicorns"; this is all about
about fixing bugs that don't exist because there is nothing there
in the real world.

In this code buffer is always something normal in size and does
not exceed 2^32-1.

Re: [dpdk-dev] [PATCH 5/5] Fix usage of fgets in various places

[Panu Matilainen]

On 02/24/2015 09:01 PM, Stephen Hemminger wrote:
> On Tue, 24 Feb 2015 11:20:33 +0200
> Panu Matilainen <pmatilai@redhat.com> wrote:
>
>> The tool is technically correct, even if loss of precision might be
>> unlikely to occur in this context
>
> Overflow is not there in the code.
> That is why I said "shooting Unicorns"; this is all about
> about fixing bugs that don't exist because there is nothing there
> in the real world.
>
> In this code buffer is always something normal in size and does
> not exceed 2^32-1.

Oh, if the tool is complaining about fixed-size buffers then yeah the 
the tool is being silly, sorry I didn't actually look up the cases.

	- Panu -

[dpdk-dev] [PATCH v2 0/4] Fix issues reported by static analysis tool

[Pawel Wodkowski]

Static analysis report some issues against current DPDK version. Most of
them need only cosmetic code changes (changing type of variable).

One issue related with ring pmd fix real memory leak problem.

PATCH v2 changes:
 - remove patch 5/5 as it was NACKed
 - reword commit log acording to mailing list sugestions.

Pawel Wodkowski (4):
  rte_timer: change declaration of rte_timer_cb_t
  librte_kvargs: make rte_kvargs_free() be consistent with other
    "free()"     functions
  pmd ring: fix possible memory leak during devinit
  cmdline: make parse_set_list() use size_t instead of int for low/high 
       parameter

 lib/librte_cmdline/cmdline_parse_portlist.c | 4 ++--
 lib/librte_kvargs/rte_kvargs.c              | 4 ++++
 lib/librte_kvargs/rte_kvargs.h              | 3 ++-
 lib/librte_pmd_ring/rte_eth_ring.c          | 6 +++---
 lib/librte_timer/rte_timer.h                | 4 ++--
 5 files changed, 13 insertions(+), 8 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v2 1/4] rte_timer: change declaration of rte_timer_cb_t

[Pawel Wodkowski]

This patch remove inconsistency between declaration of type
rte_timer_cb_t, field f in struct rte_timer and function
__rte_timer_reset().

Although compiler treat both of them the same, the static analysis tool
like complain about that.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_timer/rte_timer.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 35b8719..77547c6 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -115,7 +115,7 @@ struct rte_timer;
 /**
  * Callback function type for timer expiry.
  */
-typedef void (rte_timer_cb_t)(struct rte_timer *, void *);
+typedef void (*rte_timer_cb_t)(struct rte_timer *, void *);
 
 #define MAX_SKIPLIST_DEPTH 10
 
@@ -128,7 +128,7 @@ struct rte_timer
 	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
 	volatile union rte_timer_status status; /**< Status of timer. */
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
-	rte_timer_cb_t *f;     /**< Callback function. */
+	rte_timer_cb_t f;      /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
 };
 
-- 
1.9.1

[dpdk-dev] [PATCH v2 2/4] librte_kvargs: make rte_kvargs_free() be consistent with other "free()" functions

[Pawel Wodkowski]

By convenction free() functions should ignore NULL parameter. This patch
add this behaviour for rte_kvargs_free().

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_kvargs/rte_kvargs.c | 4 ++++
 lib/librte_kvargs/rte_kvargs.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index 8bc1e46..c2dd051 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -174,8 +174,12 @@ rte_kvargs_process(const struct rte_kvargs *kvlist,
 void
 rte_kvargs_free(struct rte_kvargs *kvlist)
 {
+	if (!kvlist)
+		return;
+
 	if (kvlist->str != NULL)
 		free(kvlist->str);
+
 	free(kvlist);
 }
 
diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h
index ef4efab..ae9ae79 100644
--- a/lib/librte_kvargs/rte_kvargs.h
+++ b/lib/librte_kvargs/rte_kvargs.h
@@ -115,7 +115,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist);
  *
  * For each key/value association that matches the given key, calls the
  * handler function with the for a given arg_name passing the value on the
- * dictionary for that key and a given extra argument.
+ * dictionary for that key and a given extra argument. If *kvlist* is NULL
+ * function does nothing.
  *
  * @param kvlist
  *   The rte_kvargs structure
-- 
1.9.1

[dpdk-dev] [PATCH v2 3/4] pmd ring: fix possible memory leak during devinit

[Pawel Wodkowski]

Free kvlist on function exit to avoid memory leak.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_pmd_ring/rte_eth_ring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_pmd_ring/rte_eth_ring.c b/lib/librte_pmd_ring/rte_eth_ring.c
index a5dc71e..f049bb3 100644
--- a/lib/librte_pmd_ring/rte_eth_ring.c
+++ b/lib/librte_pmd_ring/rte_eth_ring.c
@@ -527,7 +527,7 @@ out:
 static int
 rte_pmd_ring_devinit(const char *name, const char *params)
 {
-	struct rte_kvargs *kvlist;
+	struct rte_kvargs *kvlist = NULL;
 	int ret = 0;
 	struct node_action_list *info = NULL;
 
@@ -548,7 +548,7 @@ rte_pmd_ring_devinit(const char *name, const char *params)
 			info = rte_zmalloc("struct node_action_list", sizeof(struct node_action_list) +
 					   (sizeof(struct node_action_pair) * ret), 0);
 			if (!info)
-				goto out;
+				goto out_free;
 
 			info->total = ret;
 			info->list = (struct node_action_pair*)(info + 1);
@@ -567,8 +567,8 @@ rte_pmd_ring_devinit(const char *name, const char *params)
 	}
 
 out_free:
+	rte_kvargs_free(kvlist);
 	rte_free(info);
-out:
 	return ret;
 }
 
-- 
1.9.1

[dpdk-dev] [PATCH v2 4/4] cmdline: make parse_set_list() use size_t instead of int for low/high parameter

[Pawel Wodkowski]

Fix warning reported during static analysis about size_t to int cast
when passing
parameters to parse_set_list().

This patch fix code formating errors that give checkpatch.pl errors
after generating patch.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_cmdline/cmdline_parse_portlist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
index fc6c14e..9c1fe3e 100644
--- a/lib/librte_cmdline/cmdline_parse_portlist.c
+++ b/lib/librte_cmdline/cmdline_parse_portlist.c
@@ -78,7 +78,7 @@ struct cmdline_token_ops cmdline_token_portlist_ops = {
 };
 
 static void
-parse_set_list(cmdline_portlist_t * pl, int low, int high)
+parse_set_list(cmdline_portlist_t *pl, size_t low, size_t high)
 {
 	do {
 		pl->map |= (1 << low++);
@@ -86,7 +86,7 @@ parse_set_list(cmdline_portlist_t * pl, int low, int high)
 }
 
 static int
-parse_ports(cmdline_portlist_t * pl, const char * str)
+parse_ports(cmdline_portlist_t *pl, const char *str)
 {
 	size_t ps, pe;
 	const char *first, *last;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 0/4] Fix issues reported by static analysis tool

[Olivier MATZ]

Hi Pawel,

On 02/25/2015 01:41 PM, Pawel Wodkowski wrote:
> Static analysis report some issues against current DPDK version. Most of
> them need only cosmetic code changes (changing type of variable).
> 
> One issue related with ring pmd fix real memory leak problem.
> 
> PATCH v2 changes:
>  - remove patch 5/5 as it was NACKed
>  - reword commit log acording to mailing list sugestions.
> 
> Pawel Wodkowski (4):
>   rte_timer: change declaration of rte_timer_cb_t
>   librte_kvargs: make rte_kvargs_free() be consistent with other
>     "free()"     functions
>   pmd ring: fix possible memory leak during devinit
>   cmdline: make parse_set_list() use size_t instead of int for low/high 
>        parameter
> 
>  lib/librte_cmdline/cmdline_parse_portlist.c | 4 ++--
>  lib/librte_kvargs/rte_kvargs.c              | 4 ++++
>  lib/librte_kvargs/rte_kvargs.h              | 3 ++-
>  lib/librte_pmd_ring/rte_eth_ring.c          | 6 +++---
>  lib/librte_timer/rte_timer.h                | 4 ++--
>  5 files changed, 13 insertions(+), 8 deletions(-)
> 

Series:
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v2 0/4] Fix issues reported by static analysis tool

[Thomas Monjalon]

> > Static analysis report some issues against current DPDK version. Most of
> > them need only cosmetic code changes (changing type of variable).
> > 
> > One issue related with ring pmd fix real memory leak problem.
> > 
> > PATCH v2 changes:
> >  - remove patch 5/5 as it was NACKed
> >  - reword commit log acording to mailing list sugestions.
> > 
> > Pawel Wodkowski (4):
> >   rte_timer: change declaration of rte_timer_cb_t
> >   librte_kvargs: make rte_kvargs_free() be consistent with other
> >     "free()"     functions
> >   pmd ring: fix possible memory leak during devinit
> >   cmdline: make parse_set_list() use size_t instead of int for low/high 
> >        parameter
> 
> Series:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks