diff mbox

[dpdk-dev,v5,1/6] librte_table: fix acl entry add and delete functions

Message ID 1504802598-27296-2-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Iremonger, Bernard Sept. 7, 2017, 4:43 p.m. UTC 
  The rte_table_acl_entry_add() function was returning data from
acl_memory instead of acl_rule_memory. It was also returning data
from entry instead of entry_ptr.

The rte_table_acl_entry_delete() function was returning data from
acl_memory instead of acl_rule_memory.

Fixes: 166923eb2f78 ("table: ACL")
Cc: stable@dpdk.org
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_table/rte_table_acl.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Jasvinder Singh Sept. 18, 2017, 3:29 p.m. UTC | #1 
Hi Bernard,

<snip>

> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -316,8 +316,7 @@ struct rte_table_acl {
>  		if (status == 0) {
>  			*key_found = 1;
>  			*entry_ptr = &acl->memory[i * acl->entry_size];
> -			memcpy(*entry_ptr, entry, acl->entry_size);
> -
> +			memcpy(entry, *entry_ptr, acl->entry_size);
>  			return 0;
>  		}
>  	}

In this case, table entry which is being added already presents in the table. So, first the pointer to that entry from the memory[] that stores the  pipeline table data(struct rte_pipeline_table_entry) associated with key is retrieved and the changes (action and metadara) are stored in the internal table pointed by action_table. So, above fix doesn't seem correct.

> @@ -353,8 +352,8 @@ struct rte_table_acl {
>  		rte_acl_free(acl->ctx);
>  	acl->ctx = ctx;
>  	*key_found = 0;
> -	*entry_ptr = &acl->memory[free_pos * acl->entry_size];
> -	memcpy(*entry_ptr, entry, acl->entry_size);
> +	*entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> +	memcpy(entry, *entry_ptr, acl->entry_size);
> 
>  	return 0;
>  }
> @@ -435,7 +434,7 @@ struct rte_table_acl {
>  	acl->ctx = ctx;
>  	*key_found = 1;
>  	if (entry != NULL)
> -		memcpy(entry, &acl->memory[pos * acl->entry_size],
> +		memcpy(entry, &acl->acl_rule_memory[pos * acl-
> >entry_size],
>  			acl->entry_size);


Above fixes also seems not correct. As per documentation, *entry_ptr is intended to store the handle to the pipeline table entry containing the data associated with the current key instead of pointing to memory used to store the acl rules, etc. Please refer rte_table_acl_create() where memory is initialized and organized to stores different types of internal tables (pointed by action_table, acl_rule_list and acl_rule_memory).
Cristian Dumitrescu Sept. 20, 2017, 12:21 p.m. UTC | #2 
> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Monday, September 18, 2017 4:30 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; adrien.mazarguil@6wind.com
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and
> delete functions
> 
> Hi Bernard,
> 
> <snip>
> 
> > --- a/lib/librte_table/rte_table_acl.c
> > +++ b/lib/librte_table/rte_table_acl.c
> > @@ -316,8 +316,7 @@ struct rte_table_acl {
> >  		if (status == 0) {
> >  			*key_found = 1;
> >  			*entry_ptr = &acl->memory[i * acl->entry_size];
> > -			memcpy(*entry_ptr, entry, acl->entry_size);
> > -
> > +			memcpy(entry, *entry_ptr, acl->entry_size);
> >  			return 0;
> >  		}
> >  	}
> 
> In this case, table entry which is being added already presents in the table.
> So, first the pointer to that entry from the memory[] that stores the  pipeline
> table data(struct rte_pipeline_table_entry) associated with key is retrieved
> and the changes (action and metadara) are stored in the internal table
> pointed by action_table. So, above fix doesn't seem correct.
> 
> > @@ -353,8 +352,8 @@ struct rte_table_acl {
> >  		rte_acl_free(acl->ctx);
> >  	acl->ctx = ctx;
> >  	*key_found = 0;
> > -	*entry_ptr = &acl->memory[free_pos * acl->entry_size];
> > -	memcpy(*entry_ptr, entry, acl->entry_size);
> > +	*entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> > +	memcpy(entry, *entry_ptr, acl->entry_size);
> >
> >  	return 0;
> >  }
> > @@ -435,7 +434,7 @@ struct rte_table_acl {
> >  	acl->ctx = ctx;
> >  	*key_found = 1;
> >  	if (entry != NULL)
> > -		memcpy(entry, &acl->memory[pos * acl->entry_size],
> > +		memcpy(entry, &acl->acl_rule_memory[pos * acl-
> > >entry_size],
> >  			acl->entry_size);
> 
> 
> Above fixes also seems not correct. As per documentation, *entry_ptr is
> intended to store the handle to the pipeline table entry containing the data
> associated with the current key instead of pointing to memory used to store
> the acl rules, etc. Please refer rte_table_acl_create() where memory is
> initialized and organized to stores different types of internal tables (pointed
> by action_table, acl_rule_list and acl_rule_memory).

NACK

Fully agree with Jasvinder.

Existing code is correct, proposed code changes are wrong.
Iremonger, Bernard Sept. 29, 2017, 8:25 a.m. UTC | #3 
Hi Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Wednesday, September 20, 2017 1:21 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; adrien.mazarguil@6wind.com
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and
> delete functions
> 
> 
> 
> > -----Original Message-----
> > From: Singh, Jasvinder
> > Sent: Monday, September 18, 2017 4:30 PM
> > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> > Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; adrien.mazarguil@6wind.com
> > Cc: Iremonger, Bernard <bernard.iremonger@intel.com>;
> stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add
> > and delete functions
> >
> > Hi Bernard,
> >
> > <snip>
> >
> > > --- a/lib/librte_table/rte_table_acl.c
> > > +++ b/lib/librte_table/rte_table_acl.c
> > > @@ -316,8 +316,7 @@ struct rte_table_acl {
> > >  		if (status == 0) {
> > >  			*key_found = 1;
> > >  			*entry_ptr = &acl->memory[i * acl->entry_size];
> > > -			memcpy(*entry_ptr, entry, acl->entry_size);
> > > -
> > > +			memcpy(entry, *entry_ptr, acl->entry_size);
> > >  			return 0;
> > >  		}
> > >  	}
> >
> > In this case, table entry which is being added already presents in the table.
> > So, first the pointer to that entry from the memory[] that stores the
> > pipeline table data(struct rte_pipeline_table_entry) associated with
> > key is retrieved and the changes (action and metadara) are stored in
> > the internal table pointed by action_table. So, above fix doesn't seem
> correct.
> >
> > > @@ -353,8 +352,8 @@ struct rte_table_acl {
> > >  		rte_acl_free(acl->ctx);
> > >  	acl->ctx = ctx;
> > >  	*key_found = 0;
> > > -	*entry_ptr = &acl->memory[free_pos * acl->entry_size];
> > > -	memcpy(*entry_ptr, entry, acl->entry_size);
> > > +	*entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> > > +	memcpy(entry, *entry_ptr, acl->entry_size);
> > >
> > >  	return 0;
> > >  }
> > > @@ -435,7 +434,7 @@ struct rte_table_acl {
> > >  	acl->ctx = ctx;
> > >  	*key_found = 1;
> > >  	if (entry != NULL)
> > > -		memcpy(entry, &acl->memory[pos * acl->entry_size],
> > > +		memcpy(entry, &acl->acl_rule_memory[pos * acl-
> > > >entry_size],
> > >  			acl->entry_size);
> >
> >
> > Above fixes also seems not correct. As per documentation, *entry_ptr
> > is intended to store the handle to the pipeline table entry containing
> > the data associated with the current key instead of pointing to memory
> > used to store the acl rules, etc. Please refer rte_table_acl_create()
> > where memory is initialized and organized to stores different types of
> > internal tables (pointed by action_table, acl_rule_list and
> acl_rule_memory).
> 
> NACK
> 
> Fully agree with Jasvinder.
> 
> Existing code is correct, proposed code changes are wrong.

I will drop this patch and send a v6 patchset.

Regards,

Bernard.
diff mbox

Patch

diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
index 3c05e4a..e84b437 100644
--- a/lib/librte_table/rte_table_acl.c
+++ b/lib/librte_table/rte_table_acl.c
@@ -316,8 +316,7 @@  struct rte_table_acl {
 		if (status == 0) {
 			*key_found = 1;
 			*entry_ptr = &acl->memory[i * acl->entry_size];
-			memcpy(*entry_ptr, entry, acl->entry_size);
-
+			memcpy(entry, *entry_ptr, acl->entry_size);
 			return 0;
 		}
 	}
@@ -353,8 +352,8 @@  struct rte_table_acl {
 		rte_acl_free(acl->ctx);
 	acl->ctx = ctx;
 	*key_found = 0;
-	*entry_ptr = &acl->memory[free_pos * acl->entry_size];
-	memcpy(*entry_ptr, entry, acl->entry_size);
+	*entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
+	memcpy(entry, *entry_ptr, acl->entry_size);
 
 	return 0;
 }
@@ -435,7 +434,7 @@  struct rte_table_acl {
 	acl->ctx = ctx;
 	*key_found = 1;
 	if (entry != NULL)
-		memcpy(entry, &acl->memory[pos * acl->entry_size],
+		memcpy(entry, &acl->acl_rule_memory[pos * acl->entry_size],
 			acl->entry_size);
 
 	return 0;