diff mbox

[dpdk-dev,v1,1/6] librte_table: move structure to header file

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

Checks

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

Commit Message

Bernard Iremonger Aug. 23, 2017, 1:51 p.m. UTC
Move struct librte_table from the rte_table_acl.c to
the rte_table_acl.h file.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_table/rte_table_acl.c | 24 ------------------------
 lib/librte_table/rte_table_acl.h | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 24 deletions(-)

Comments

Dumitrescu, Cristian Aug. 23, 2017, 2:13 p.m. UTC | #1
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, August 23, 2017 2:51 PM
> To: 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>
> Subject: [PATCH v1 1/6] librte_table: move structure to header file
> 
> Move struct librte_table from the rte_table_acl.c to
> the rte_table_acl.h file.
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  lib/librte_table/rte_table_acl.c | 24 ------------------------
>  lib/librte_table/rte_table_acl.h | 24 ++++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
> index 3c05e4a..900f658 100644
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -57,30 +57,6 @@
> 
>  #endif
> 
> -struct rte_table_acl {
> -	struct rte_table_stats stats;
> -
> -	/* Low-level ACL table */
> -	char name[2][RTE_ACL_NAMESIZE];
> -	struct rte_acl_param acl_params; /* for creating low level acl table */
> -	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> -	struct rte_acl_ctx *ctx;
> -	uint32_t name_id;
> -
> -	/* Input parameters */
> -	uint32_t n_rules;
> -	uint32_t entry_size;
> -
> -	/* Internal tables */
> -	uint8_t *action_table;
> -	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> -	uint8_t *acl_rule_memory; /* Memory to store the rules */
> -
> -	/* Memory to store the action table and stack of free entries */
> -	uint8_t memory[0] __rte_cache_aligned;
> -};
> -
> -
>  static void *
>  rte_table_acl_create(
>  	void *params,
> diff --git a/lib/librte_table/rte_table_acl.h b/lib/librte_table/rte_table_acl.h
> index a9cc032..1370b12 100644
> --- a/lib/librte_table/rte_table_acl.h
> +++ b/lib/librte_table/rte_table_acl.h
> @@ -55,6 +55,30 @@
> 
>  #include "rte_table.h"
> 
> +
> +struct rte_table_acl {
> +	struct rte_table_stats stats;
> +
> +	/* Low-level ACL table */
> +	char name[2][RTE_ACL_NAMESIZE];
> +	struct rte_acl_param acl_params; /* for creating low level acl table */
> +	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> +	struct rte_acl_ctx *ctx;
> +	uint32_t name_id;
> +
> +	/* Input parameters */
> +	uint32_t n_rules;
> +	uint32_t entry_size;
> +
> +	/* Internal tables */
> +	uint8_t *action_table;
> +	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> +	uint8_t *acl_rule_memory; /* Memory to store the rules */
> +
> +	/* Memory to store the action table and stack of free entries */
> +	uint8_t memory[0] __rte_cache_aligned;
> +};
> +
>  /** ACL table parameters */
>  struct rte_table_acl_params {
>  	/** Name */
> --
> 1.9.1


Hi Bernard,

Strong objection here:
- This data structure contains the internal data needed to run the ACL table. It is implementation dependent, it is not part of the API. Therefore, it must not be exposed as part of the API, so it has to stay in the .c file as opposed to the .h file.
- Users should handle the ACL table through the handle returned by the create function as opposed to accessing this structure directly.

Regards,
Cristian
Bernard Iremonger Aug. 23, 2017, 2:32 p.m. UTC | #2
Hi Cristian,

<snip>

> > Subject: [PATCH v1 1/6] librte_table: move structure to header file
> >
> > Move struct librte_table from the rte_table_acl.c to the
> > rte_table_acl.h file.
> >
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  lib/librte_table/rte_table_acl.c | 24 ------------------------
> > lib/librte_table/rte_table_acl.h | 24 ++++++++++++++++++++++++
> >  2 files changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/lib/librte_table/rte_table_acl.c
> > b/lib/librte_table/rte_table_acl.c
> > index 3c05e4a..900f658 100644
> > --- a/lib/librte_table/rte_table_acl.c
> > +++ b/lib/librte_table/rte_table_acl.c
> > @@ -57,30 +57,6 @@
> >
> >  #endif
> >
> > -struct rte_table_acl {
> > -	struct rte_table_stats stats;
> > -
> > -	/* Low-level ACL table */
> > -	char name[2][RTE_ACL_NAMESIZE];
> > -	struct rte_acl_param acl_params; /* for creating low level acl table */
> > -	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> > -	struct rte_acl_ctx *ctx;
> > -	uint32_t name_id;
> > -
> > -	/* Input parameters */
> > -	uint32_t n_rules;
> > -	uint32_t entry_size;
> > -
> > -	/* Internal tables */
> > -	uint8_t *action_table;
> > -	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> > -	uint8_t *acl_rule_memory; /* Memory to store the rules */
> > -
> > -	/* Memory to store the action table and stack of free entries */
> > -	uint8_t memory[0] __rte_cache_aligned;
> > -};
> > -
> > -
> >  static void *
> >  rte_table_acl_create(
> >  	void *params,
> > diff --git a/lib/librte_table/rte_table_acl.h
> > b/lib/librte_table/rte_table_acl.h
> > index a9cc032..1370b12 100644
> > --- a/lib/librte_table/rte_table_acl.h
> > +++ b/lib/librte_table/rte_table_acl.h
> > @@ -55,6 +55,30 @@
> >
> >  #include "rte_table.h"
> >
> > +
> > +struct rte_table_acl {
> > +	struct rte_table_stats stats;
> > +
> > +	/* Low-level ACL table */
> > +	char name[2][RTE_ACL_NAMESIZE];
> > +	struct rte_acl_param acl_params; /* for creating low level acl table */
> > +	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> > +	struct rte_acl_ctx *ctx;
> > +	uint32_t name_id;
> > +
> > +	/* Input parameters */
> > +	uint32_t n_rules;
> > +	uint32_t entry_size;
> > +
> > +	/* Internal tables */
> > +	uint8_t *action_table;
> > +	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> > +	uint8_t *acl_rule_memory; /* Memory to store the rules */
> > +
> > +	/* Memory to store the action table and stack of free entries */
> > +	uint8_t memory[0] __rte_cache_aligned; };
> > +
> >  /** ACL table parameters */
> >  struct rte_table_acl_params {
> >  	/** Name */
> > --
> > 1.9.1
> 
> 
> Hi Bernard,
> 
> Strong objection here:
> - This data structure contains the internal data needed to run the ACL table. It
> is implementation dependent, it is not part of the API. Therefore, it must not
> be exposed as part of the API, so it has to stay in the .c file as opposed to the
> .h file.
> - Users should handle the ACL table through the handle returned by the
> create function as opposed to accessing this structure directly.
> 
> Regards,
> Cristian

I will revisit this to see if there is another way.

Regards,

Bernard.
Bernard Iremonger Aug. 28, 2017, 8:48 a.m. UTC | #3
Hi Cristian,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Wednesday, August 23, 2017 3:32 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; adrien.mazarguil@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v1 1/6] librte_table: move structure to
> header file
> 
> Hi Cristian,
> 
> <snip>
> 
> > > Subject: [PATCH v1 1/6] librte_table: move structure to header file
> > >
> > > Move struct librte_table from the rte_table_acl.c to the
> > > rte_table_acl.h file.
> > >
> > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > > ---
> > >  lib/librte_table/rte_table_acl.c | 24 ------------------------
> > > lib/librte_table/rte_table_acl.h | 24 ++++++++++++++++++++++++
> > >  2 files changed, 24 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/lib/librte_table/rte_table_acl.c
> > > b/lib/librte_table/rte_table_acl.c
> > > index 3c05e4a..900f658 100644
> > > --- a/lib/librte_table/rte_table_acl.c
> > > +++ b/lib/librte_table/rte_table_acl.c
> > > @@ -57,30 +57,6 @@
> > >
> > >  #endif
> > >
> > > -struct rte_table_acl {
> > > -	struct rte_table_stats stats;
> > > -
> > > -	/* Low-level ACL table */
> > > -	char name[2][RTE_ACL_NAMESIZE];
> > > -	struct rte_acl_param acl_params; /* for creating low level acl table */
> > > -	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> > > -	struct rte_acl_ctx *ctx;
> > > -	uint32_t name_id;
> > > -
> > > -	/* Input parameters */
> > > -	uint32_t n_rules;
> > > -	uint32_t entry_size;
> > > -
> > > -	/* Internal tables */
> > > -	uint8_t *action_table;
> > > -	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> > > -	uint8_t *acl_rule_memory; /* Memory to store the rules */
> > > -
> > > -	/* Memory to store the action table and stack of free entries */
> > > -	uint8_t memory[0] __rte_cache_aligned;
> > > -};
> > > -
> > > -
> > >  static void *
> > >  rte_table_acl_create(
> > >  	void *params,
> > > diff --git a/lib/librte_table/rte_table_acl.h
> > > b/lib/librte_table/rte_table_acl.h
> > > index a9cc032..1370b12 100644
> > > --- a/lib/librte_table/rte_table_acl.h
> > > +++ b/lib/librte_table/rte_table_acl.h
> > > @@ -55,6 +55,30 @@
> > >
> > >  #include "rte_table.h"
> > >
> > > +
> > > +struct rte_table_acl {
> > > +	struct rte_table_stats stats;
> > > +
> > > +	/* Low-level ACL table */
> > > +	char name[2][RTE_ACL_NAMESIZE];
> > > +	struct rte_acl_param acl_params; /* for creating low level acl table */
> > > +	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
> > > +	struct rte_acl_ctx *ctx;
> > > +	uint32_t name_id;
> > > +
> > > +	/* Input parameters */
> > > +	uint32_t n_rules;
> > > +	uint32_t entry_size;
> > > +
> > > +	/* Internal tables */
> > > +	uint8_t *action_table;
> > > +	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
> > > +	uint8_t *acl_rule_memory; /* Memory to store the rules */
> > > +
> > > +	/* Memory to store the action table and stack of free entries */
> > > +	uint8_t memory[0] __rte_cache_aligned; };
> > > +
> > >  /** ACL table parameters */
> > >  struct rte_table_acl_params {
> > >  	/** Name */
> > > --
> > > 1.9.1
> >
> >
> > Hi Bernard,
> >
> > Strong objection here:
> > - This data structure contains the internal data needed to run the ACL
> > table. It is implementation dependent, it is not part of the API.
> > Therefore, it must not be exposed as part of the API, so it has to
> > stay in the .c file as opposed to the .h file.
> > - Users should handle the ACL table through the handle returned by the
> > create function as opposed to accessing this structure directly.
> >
> > Regards,
> > Cristian
> 
> I will revisit this to see if there is another way.
> 
> Regards,
> 
> Bernard.
> 

This patch has been dropped from the v2 patch set.
The functionality needed has been implemented in a different way.

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..900f658 100644
--- a/lib/librte_table/rte_table_acl.c
+++ b/lib/librte_table/rte_table_acl.c
@@ -57,30 +57,6 @@ 
 
 #endif
 
-struct rte_table_acl {
-	struct rte_table_stats stats;
-
-	/* Low-level ACL table */
-	char name[2][RTE_ACL_NAMESIZE];
-	struct rte_acl_param acl_params; /* for creating low level acl table */
-	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
-	struct rte_acl_ctx *ctx;
-	uint32_t name_id;
-
-	/* Input parameters */
-	uint32_t n_rules;
-	uint32_t entry_size;
-
-	/* Internal tables */
-	uint8_t *action_table;
-	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
-	uint8_t *acl_rule_memory; /* Memory to store the rules */
-
-	/* Memory to store the action table and stack of free entries */
-	uint8_t memory[0] __rte_cache_aligned;
-};
-
-
 static void *
 rte_table_acl_create(
 	void *params,
diff --git a/lib/librte_table/rte_table_acl.h b/lib/librte_table/rte_table_acl.h
index a9cc032..1370b12 100644
--- a/lib/librte_table/rte_table_acl.h
+++ b/lib/librte_table/rte_table_acl.h
@@ -55,6 +55,30 @@ 
 
 #include "rte_table.h"
 
+
+struct rte_table_acl {
+	struct rte_table_stats stats;
+
+	/* Low-level ACL table */
+	char name[2][RTE_ACL_NAMESIZE];
+	struct rte_acl_param acl_params; /* for creating low level acl table */
+	struct rte_acl_config cfg; /* Holds the field definitions (metadata) */
+	struct rte_acl_ctx *ctx;
+	uint32_t name_id;
+
+	/* Input parameters */
+	uint32_t n_rules;
+	uint32_t entry_size;
+
+	/* Internal tables */
+	uint8_t *action_table;
+	struct rte_acl_rule **acl_rule_list; /* Array of pointers to rules */
+	uint8_t *acl_rule_memory; /* Memory to store the rules */
+
+	/* Memory to store the action table and stack of free entries */
+	uint8_t memory[0] __rte_cache_aligned;
+};
+
 /** ACL table parameters */
 struct rte_table_acl_params {
 	/** Name */