[dpdk-dev] [PATCH 0/5] fixup ip pipeline examples

[dpdk-dev] [PATCH 0/5] fixup ip pipeline examples

[Stephen Hemminger]

Lots of little trivial bugs/typos here.
Let's not start users off with a bad example.

Stephen Hemminger (5):
  examples_ip_pipeline: fix typo's
  example_ip_pipeline: avoid strncpy issue
  example_ip_pipeline: fix sizeof() on memcpy
  examples_ip_pipeline: remove useless code
  examples_ip_pipeline: fix possible string overrun

 examples/ip_pipeline/app.h                                   | 2 +-
 examples/ip_pipeline/config_parse.c                          | 5 +----
 examples/ip_pipeline/config_parse_tm.c                       | 6 ++++--
 examples/ip_pipeline/init.c                                  | 7 ++++---
 examples/ip_pipeline/pipeline/pipeline_common_fe.c           | 2 +-
 examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 3 +--
 6 files changed, 12 insertions(+), 13 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH 1/5] examples_ip_pipeline: fix typo's

[Stephen Hemminger]

Coverity found these as dead-code and/or copy-paste bugs.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/config_parse.c                          | 2 +-
 examples/ip_pipeline/config_parse_tm.c                       | 2 +-
 examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/examples/ip_pipeline/config_parse.c b/examples/ip_pipeline/config_parse.c
index c9b78f9..6b651a8 100644
--- a/examples/ip_pipeline/config_parse.c
+++ b/examples/ip_pipeline/config_parse.c
@@ -2238,7 +2238,7 @@ save_pipeline_params(struct app_params *app, FILE *f)
 		}
 
 		/* msgq_out */
-		if (p->n_msgq_in) {
+		if (p->n_msgq_out) {
 			uint32_t j;
 
 			fprintf(f, "msgq_out =");
diff --git a/examples/ip_pipeline/config_parse_tm.c b/examples/ip_pipeline/config_parse_tm.c
index cdebbdc..84702b0 100644
--- a/examples/ip_pipeline/config_parse_tm.c
+++ b/examples/ip_pipeline/config_parse_tm.c
@@ -399,7 +399,7 @@ tm_cfgfile_load(struct app_pktq_tm_params *tm)
 
 	memset(tm->sched_subport_params, 0, sizeof(tm->sched_subport_params));
 	memset(tm->sched_pipe_profiles, 0, sizeof(tm->sched_pipe_profiles));
-	memset(&tm->sched_port_params, 0, sizeof(tm->sched_pipe_profiles));
+	memset(&tm->sched_port_params, 0, sizeof(tm->sched_port_params));
 	for (i = 0; i < APP_MAX_SCHED_SUBPORTS * APP_MAX_SCHED_PIPES; i++)
 		tm->sched_pipe_to_profile[i] = -1;
 
diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
index 4b82180..24cf7dc 100644
--- a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
+++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
@@ -442,7 +442,7 @@ app_pipeline_fc_add_bulk(struct app_params *app,
 	flow_rsp = rte_malloc(NULL,
 		n_keys * sizeof(struct pipeline_fc_add_bulk_flow_rsp),
 		RTE_CACHE_LINE_SIZE);
-	if (flow_req == NULL) {
+	if (flow_rsq == NULL) {
 		rte_free(flow_req);
 		rte_free(new_flow);
 		rte_free(signature);
-- 
2.1.4

[dpdk-dev] [PATCH 2/5] example_ip_pipeline: avoid strncpy issue

[Stephen Hemminger]

If name is so long that it fills buffer, then string would not
be null terminated.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/config_parse_tm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/examples/ip_pipeline/config_parse_tm.c b/examples/ip_pipeline/config_parse_tm.c
index 84702b0..4a35715 100644
--- a/examples/ip_pipeline/config_parse_tm.c
+++ b/examples/ip_pipeline/config_parse_tm.c
@@ -354,7 +354,9 @@ tm_cfgfile_load_sched_subport(
 					profile = atoi(entries[j].value);
 					strncpy(name,
 						entries[j].name,
-						sizeof(name));
+						CFG_NAME_LEN - 1);
+					name[CFG_NAME_LEN-1] = '\0';
+
 					n_tokens = rte_strsplit(
 						&name[sizeof("pipe")],
 						strnlen(name, CFG_NAME_LEN),
-- 
2.1.4

[dpdk-dev] [PATCH 3/5] example_ip_pipeline: fix sizeof() on memcpy

[Stephen Hemminger]

Found by Coverity:
  Sizeof not portable (SIZEOF_MISMATCH)
  suspicious_sizeof: Passing argument &app->cmds[app->n_cmds] of type
  cmdline_parse_ctx_t * and argument n_cmds * 8UL /* sizeof (cmdline_parse_ctx_t *) */
  to function memcpy is suspicious.
   In this case, sizeof (cmdline_parse_ctx_t *) is equal to sizeof (cmdline_parse_ctx_t),
   but this is not a portable assumption.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/init.c                                  | 2 +-
 examples/ip_pipeline/pipeline/pipeline_common_fe.c           | 2 +-
 examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
index 3f9c68d..75e3767 100644
--- a/examples/ip_pipeline/init.c
+++ b/examples/ip_pipeline/init.c
@@ -1325,7 +1325,7 @@ app_pipeline_type_cmd_push(struct app_params *app,
 	/* Push pipeline commands into the application */
 	memcpy(&app->cmds[app->n_cmds],
 		cmds,
-		n_cmds * sizeof(cmdline_parse_ctx_t *));
+		n_cmds * sizeof(cmdline_parse_ctx_t));
 
 	for (i = 0; i < n_cmds; i++)
 		app->cmds[app->n_cmds + i]->data = app;
diff --git a/examples/ip_pipeline/pipeline/pipeline_common_fe.c b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
index fcda0ce..4eec66b 100644
--- a/examples/ip_pipeline/pipeline/pipeline_common_fe.c
+++ b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
@@ -1321,7 +1321,7 @@ app_pipeline_common_cmd_push(struct app_params *app)
 	/* Push pipeline commands into the application */
 	memcpy(&app->cmds[app->n_cmds],
 		pipeline_common_cmds,
-		n_cmds * sizeof(cmdline_parse_ctx_t *));
+		n_cmds * sizeof(cmdline_parse_ctx_t));
 
 	for (i = 0; i < n_cmds; i++)
 		app->cmds[app->n_cmds + i]->data = app;
diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
index 24cf7dc..e5141b0 100644
--- a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
+++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
@@ -126,7 +126,6 @@ app_pipeline_fc_key_convert(struct pipeline_fc_key *key_in,
 	{
 		struct pkt_key_ipv6_5tuple *ipv6 = key_buffer;
 
-		memset(ipv6, 0, 64);
 		ipv6->payload_length = 0;
 		ipv6->proto = key_in->key.ipv6_5tuple.proto;
 		ipv6->hop_limit = 0;
-- 
2.1.4

[dpdk-dev] [PATCH 4/5] examples_ip_pipeline: remove useless code

[Stephen Hemminger]

Code here checks return from strdup() in only one place in this
whole module, and then does nothing useful by setting one
value that is then cleared. Just remove the check.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/config_parse.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/examples/ip_pipeline/config_parse.c b/examples/ip_pipeline/config_parse.c
index 6b651a8..81ec33b 100644
--- a/examples/ip_pipeline/config_parse.c
+++ b/examples/ip_pipeline/config_parse.c
@@ -1483,9 +1483,6 @@ parse_tm(struct app_params *app,
 		ret = -ESRCH;
 		if (strcmp(ent->name, "cfg") == 0) {
 			param->file_name = strdup(ent->value);
-			if (param->file_name == NULL)
-				ret = -EINVAL;
-
 			ret = 0;
 		} else if (strcmp(ent->name, "burst_read") == 0)
 			ret = parser_read_uint32(&param->burst_read,
-- 
2.1.4

[dpdk-dev] [PATCH 5/5] examples_ip_pipeline: fix possible string overrun

[Stephen Hemminger]

If a long name was passed the code would clobber memory with
strcpy.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/app.h  | 2 +-
 examples/ip_pipeline/init.c | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/examples/ip_pipeline/app.h b/examples/ip_pipeline/app.h
index 521e3a0..1f6bf0c 100644
--- a/examples/ip_pipeline/app.h
+++ b/examples/ip_pipeline/app.h
@@ -190,7 +190,7 @@ struct app_pktq_out_params {
 #define APP_MAX_PIPELINE_ARGS                    PIPELINE_MAX_ARGS
 
 struct app_pipeline_params {
-	char *name;
+	const char *name;
 	uint8_t parsed;
 
 	char type[APP_PIPELINE_TYPE_SIZE];
diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
index 75e3767..007af83 100644
--- a/examples/ip_pipeline/init.c
+++ b/examples/ip_pipeline/init.c
@@ -1022,12 +1022,13 @@ app_init_msgq(struct app_params *app)
 }
 
 static void app_pipeline_params_get(struct app_params *app,
-	struct app_pipeline_params *p_in,
+	const struct app_pipeline_params *p_in,
 	struct pipeline_params *p_out)
 {
 	uint32_t i;
 
-	strcpy(p_out->name, p_in->name);
+	strncpy(p_out->name, p_in->name, PIPELINE_NAME_SIZE - 1);
+	p_out->name[PIPELINE_NAME_SIZE - 1] = '\0';
 
 	p_out->socket_id = (int) p_in->socket_id;
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/5] examples_ip_pipeline: fix typo's

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 1/5] examples_ip_pipeline: fix typo's
> 
> Coverity found these as dead-code and/or copy-paste bugs.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/config_parse.c                          | 2 +-
>  examples/ip_pipeline/config_parse_tm.c                       | 2 +-
>  examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ip_pipeline/config_parse.c
> b/examples/ip_pipeline/config_parse.c
> index c9b78f9..6b651a8 100644
> --- a/examples/ip_pipeline/config_parse.c
> +++ b/examples/ip_pipeline/config_parse.c
> @@ -2238,7 +2238,7 @@ save_pipeline_params(struct app_params *app,
> FILE *f)
>  		}
> 
>  		/* msgq_out */
> -		if (p->n_msgq_in) {
> +		if (p->n_msgq_out) {
>  			uint32_t j;
> 
>  			fprintf(f, "msgq_out =");
> diff --git a/examples/ip_pipeline/config_parse_tm.c
> b/examples/ip_pipeline/config_parse_tm.c
> index cdebbdc..84702b0 100644
> --- a/examples/ip_pipeline/config_parse_tm.c
> +++ b/examples/ip_pipeline/config_parse_tm.c
> @@ -399,7 +399,7 @@ tm_cfgfile_load(struct app_pktq_tm_params *tm)
> 
>  	memset(tm->sched_subport_params, 0, sizeof(tm-
> >sched_subport_params));
>  	memset(tm->sched_pipe_profiles, 0, sizeof(tm-
> >sched_pipe_profiles));
> -	memset(&tm->sched_port_params, 0, sizeof(tm-
> >sched_pipe_profiles));
> +	memset(&tm->sched_port_params, 0, sizeof(tm-
> >sched_port_params));
>  	for (i = 0; i < APP_MAX_SCHED_SUBPORTS *
> APP_MAX_SCHED_PIPES; i++)
>  		tm->sched_pipe_to_profile[i] = -1;
> 
> diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> index 4b82180..24cf7dc 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> @@ -442,7 +442,7 @@ app_pipeline_fc_add_bulk(struct app_params *app,
>  	flow_rsp = rte_malloc(NULL,
>  		n_keys * sizeof(struct pipeline_fc_add_bulk_flow_rsp),
>  		RTE_CACHE_LINE_SIZE);
> -	if (flow_req == NULL) {
> +	if (flow_rsq == NULL) {
>  		rte_free(flow_req);
>  		rte_free(new_flow);
>  		rte_free(signature);
> --
> 2.1.4

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [dpdk-dev] [PATCH 2/5] example_ip_pipeline: avoid strncpy issue

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 2/5] example_ip_pipeline: avoid strncpy issue
> 
> If name is so long that it fills buffer, then string would not
> be null terminated.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/config_parse_tm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/examples/ip_pipeline/config_parse_tm.c
> b/examples/ip_pipeline/config_parse_tm.c
> index 84702b0..4a35715 100644
> --- a/examples/ip_pipeline/config_parse_tm.c
> +++ b/examples/ip_pipeline/config_parse_tm.c
> @@ -354,7 +354,9 @@ tm_cfgfile_load_sched_subport(
>  					profile = atoi(entries[j].value);
>  					strncpy(name,
>  						entries[j].name,
> -						sizeof(name));
> +						CFG_NAME_LEN - 1);
> +					name[CFG_NAME_LEN-1] = '\0';
> +
>  					n_tokens = rte_strsplit(
>  						&name[sizeof("pipe")],
>  						strnlen(name,
> CFG_NAME_LEN),
> --
> 2.1.4

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [dpdk-dev] [PATCH 3/5] example_ip_pipeline: fix sizeof() on memcpy

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 3/5] example_ip_pipeline: fix sizeof() on memcpy
> 
> Found by Coverity:
>   Sizeof not portable (SIZEOF_MISMATCH)
>   suspicious_sizeof: Passing argument &app->cmds[app->n_cmds] of type
>   cmdline_parse_ctx_t * and argument n_cmds * 8UL /* sizeof
> (cmdline_parse_ctx_t *) */
>   to function memcpy is suspicious.
>    In this case, sizeof (cmdline_parse_ctx_t *) is equal to sizeof
> (cmdline_parse_ctx_t),
>    but this is not a portable assumption.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/init.c                                  | 2 +-
>  examples/ip_pipeline/pipeline/pipeline_common_fe.c           | 2 +-
>  examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 1 -
>  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> index 3f9c68d..75e3767 100644
> --- a/examples/ip_pipeline/init.c
> +++ b/examples/ip_pipeline/init.c
> @@ -1325,7 +1325,7 @@ app_pipeline_type_cmd_push(struct app_params
> *app,
>  	/* Push pipeline commands into the application */
>  	memcpy(&app->cmds[app->n_cmds],
>  		cmds,
> -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> +		n_cmds * sizeof(cmdline_parse_ctx_t));

Actually no, as we are both the destination and the source of memcpy are array of pointers.

The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

> 
>  	for (i = 0; i < n_cmds; i++)
>  		app->cmds[app->n_cmds + i]->data = app;
> diff --git a/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> index fcda0ce..4eec66b 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> @@ -1321,7 +1321,7 @@ app_pipeline_common_cmd_push(struct
> app_params *app)
>  	/* Push pipeline commands into the application */
>  	memcpy(&app->cmds[app->n_cmds],
>  		pipeline_common_cmds,
> -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> +		n_cmds * sizeof(cmdline_parse_ctx_t));

Actually no, as we are both the destination and the source of memcpy are array of pointers.

The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

> 
>  	for (i = 0; i < n_cmds; i++)
>  		app->cmds[app->n_cmds + i]->data = app;
> diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> index 24cf7dc..e5141b0 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> @@ -126,7 +126,6 @@ app_pipeline_fc_key_convert(struct pipeline_fc_key
> *key_in,
>  	{
>  		struct pkt_key_ipv6_5tuple *ipv6 = key_buffer;
> 
> -		memset(ipv6, 0, 64);

Agreed!

>  		ipv6->payload_length = 0;
>  		ipv6->proto = key_in->key.ipv6_5tuple.proto;
>  		ipv6->hop_limit = 0;
> --
> 2.1.4

Re: [dpdk-dev] [PATCH 4/5] examples_ip_pipeline: remove useless code

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 4/5] examples_ip_pipeline: remove useless code
> 
> Code here checks return from strdup() in only one place in this
> whole module, and then does nothing useful by setting one
> value that is then cleared. Just remove the check.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/config_parse.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/examples/ip_pipeline/config_parse.c
> b/examples/ip_pipeline/config_parse.c
> index 6b651a8..81ec33b 100644
> --- a/examples/ip_pipeline/config_parse.c
> +++ b/examples/ip_pipeline/config_parse.c
> @@ -1483,9 +1483,6 @@ parse_tm(struct app_params *app,
>  		ret = -ESRCH;
>  		if (strcmp(ent->name, "cfg") == 0) {
>  			param->file_name = strdup(ent->value);
> -			if (param->file_name == NULL)
> -				ret = -EINVAL;
> -
>  			ret = 0;

The required implementation would actually be:

	if (param->file_name == NULL)
		ret = -EINVAL;
	else
		ret = 0;

The TM parameter file_name is used by function app_config_parse_tm() in file config_parse_tm.c.

Regarding the way the error management (including the error messages) is done in our parser (currently done by repeatedly setting the ret variable), I would like to improve (replace) when I get the time to take another look at this.

>  		} else if (strcmp(ent->name, "burst_read") == 0)
>  			ret = parser_read_uint32(&param->burst_read,
> --
> 2.1.4

Re: [dpdk-dev] [PATCH 5/5] examples_ip_pipeline: fix possible string overrun

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 5/5] examples_ip_pipeline: fix possible string overrun
> 
> If a long name was passed the code would clobber memory with
> strcpy.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/app.h  | 2 +-
>  examples/ip_pipeline/init.c | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ip_pipeline/app.h b/examples/ip_pipeline/app.h
> index 521e3a0..1f6bf0c 100644
> --- a/examples/ip_pipeline/app.h
> +++ b/examples/ip_pipeline/app.h
> @@ -190,7 +190,7 @@ struct app_pktq_out_params {
>  #define APP_MAX_PIPELINE_ARGS                    PIPELINE_MAX_ARGS
> 
>  struct app_pipeline_params {
> -	char *name;
> +	const char *name;
>  	uint8_t parsed;
> 
>  	char type[APP_PIPELINE_TYPE_SIZE];
> diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> index 75e3767..007af83 100644
> --- a/examples/ip_pipeline/init.c
> +++ b/examples/ip_pipeline/init.c
> @@ -1022,12 +1022,13 @@ app_init_msgq(struct app_params *app)
>  }
> 
>  static void app_pipeline_params_get(struct app_params *app,
> -	struct app_pipeline_params *p_in,
> +	const struct app_pipeline_params *p_in,
>  	struct pipeline_params *p_out)
>  {
>  	uint32_t i;
> 
> -	strcpy(p_out->name, p_in->name);
> +	strncpy(p_out->name, p_in->name, PIPELINE_NAME_SIZE - 1);
> +	p_out->name[PIPELINE_NAME_SIZE - 1] = '\0';

Could be done, but not necessary, as the pipeline name string was already validated in the parser module; now it is just copied over, and it is safe to assume it of right size.

I don't mind doing it for extra safety.

> 
>  	p_out->socket_id = (int) p_in->socket_id;
> 
> --
> 2.1.4

Re: [dpdk-dev] [PATCH 0/5] fixup ip pipeline examples

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 0/5] fixup ip pipeline examples
> 
> Lots of little trivial bugs/typos here.
> Let's not start users off with a bad example.
> 

Thanks very much for doing this work, Steve!

I agree with most of the bug fixes here except a few, which I indicated in my reply to each individual patch.

Do you want to resend or do you want us to integrate the fixes in our next patches? Whatever works best for you is fine with us.

Regards,
Cristian

Re: [dpdk-dev] [PATCH 3/5] example_ip_pipeline: fix sizeof() on memcpy

[Stephen Hemminger]

On Wed, 9 Sep 2015 18:25:53 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> > diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> > index 3f9c68d..75e3767 100644
> > --- a/examples/ip_pipeline/init.c
> > +++ b/examples/ip_pipeline/init.c
> > @@ -1325,7 +1325,7 @@ app_pipeline_type_cmd_push(struct app_params
> > *app,
> >  	/* Push pipeline commands into the application */
> >  	memcpy(&app->cmds[app->n_cmds],
> >  		cmds,
> > -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> > +		n_cmds * sizeof(cmdline_parse_ctx_t));  
> 
> Actually no, as we are both the destination and the source of memcpy are array of pointers.
> 
> The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

In order to make tools happy, shouldn't the source and target be pointers to array of pointers.
In the current code
  &app->cmd[app->n_cmds] is type cmdline_parse_ctx_t *
  cmds is type cmdline_parse_ctx_t *

And type cmdline_parse_ctx_t is already a pointer.

Copying a set of pointers to pointers vs set of pointers will be the same since both are
the same size, but static checking tools see the problem.

This is why kernel developers particularly despise typedefs which hide
pointers like this one.

Re: [dpdk-dev] [PATCH 2/5] example_ip_pipeline: avoid strncpy issue

[Bruce Richardson]

On Mon, Aug 31, 2015 at 06:59:03PM -0700, Stephen Hemminger wrote:
> If name is so long that it fills buffer, then string would not
> be null terminated.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/config_parse_tm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/examples/ip_pipeline/config_parse_tm.c b/examples/ip_pipeline/config_parse_tm.c
> index 84702b0..4a35715 100644
> --- a/examples/ip_pipeline/config_parse_tm.c
> +++ b/examples/ip_pipeline/config_parse_tm.c
> @@ -354,7 +354,9 @@ tm_cfgfile_load_sched_subport(
>  					profile = atoi(entries[j].value);
>  					strncpy(name,
>  						entries[j].name,
> -						sizeof(name));
> +						CFG_NAME_LEN - 1);
> +					name[CFG_NAME_LEN-1] = '\0';
> +
>  					n_tokens = rte_strsplit(
>  						&name[sizeof("pipe")],
>  						strnlen(name, CFG_NAME_LEN),
> -- 
> 2.1.4
> 
Would using snprintf rather than strncpy be tidier? Would save having to worry
about null termination at all.

/Bruce

Re: [dpdk-dev] [PATCH 0/5] fixup ip pipeline examples

[Thomas Monjalon]

Hi,

2015-09-09 18:35, Dumitrescu, Cristian:
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Lots of little trivial bugs/typos here.
> > Let's not start users off with a bad example.
> 
> Thanks very much for doing this work, Steve!
> 
> I agree with most of the bug fixes here except a few, which I indicated in my reply to each individual patch.
> 
> Do you want to resend or do you want us to integrate the fixes in our next patches? Whatever works best for you is fine with us.

Please, what are the news about this series?
In case it's already merged in another series, please advertise it.
Thanks

Re: [dpdk-dev] [PATCH 0/5] fixup ip pipeline examples

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 7, 2015 5:43 PM
> To: Dumitrescu, Cristian; Stephen Hemminger
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/5] fixup ip pipeline examples
> 
> Hi,
> 
> 2015-09-09 18:35, Dumitrescu, Cristian:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Lots of little trivial bugs/typos here.
> > > Let's not start users off with a bad example.
> >
> > Thanks very much for doing this work, Steve!
> >
> > I agree with most of the bug fixes here except a few, which I indicated in
> my reply to each individual patch.
> >
> > Do you want to resend or do you want us to integrate the fixes in our next
> patches? Whatever works best for you is fine with us.
> 
> Please, what are the news about this series?
> In case it's already merged in another series, please advertise it.
> Thanks

Stephen, are you planning to send a new revision of this series? Thanks, Cristian