Re: [PATCH net-next 2/2] net: Change TCA_ACT_* to TCA_ID_* to match that of TCA_ID_POLICE

2019-02-10 Thread Eli Cohen 
On Thu, Feb 07, 2019 at 10:01:10AM -0800, David Miller wrote:
> From: Eli Cohen 
> Date: Thu,  7 Feb 2019 09:45:49 +0200
> 
> > diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> > index 902957beceb3..d54cb608dbaf 100644
> > --- a/net/sched/act_simple.c
> > +++ b/net/sched/act_simple.c
> > @@ -19,8 +19,6 @@
> >  #include 
> >  #include 
> >  
> > -#define TCA_ACT_SIMP 22
> > -
> >  #include 
> >  #include 
> >  
> 
> I would do this in patch #1.

Sure, that was the intention. It just slipped off my fingers.

Will fix and send a new series.

> Actually, because you didn't, after patch #1 there are two #defines
> evaluated for this macro.  One in pkt_cls.h and one here.
> 
> The only reason the compiler doesn't warn and complain is because the
> definitions are identical.
> 
> Thank you.

Re: [PATCH net-next 2/2] net: Change TCA_ACT_* to TCA_ID_* to match that of TCA_ID_POLICE

2019-02-08 Thread Jiri Pirko 
Fri, Feb 08, 2019 at 09:01:42AM CET, simon.hor...@netronome.com wrote:
>On Thu, Feb 07, 2019 at 09:45:49AM +0200, Eli Cohen wrote:
>> Modify the kernel users of the TCA_ACT_* macros to use TCA_ID_*. For
>> example, use TCA_ID_GACT instead of TCA_ACT_GACT. This will align with
>> TCA_ID_POLICE and also differentiates these identifier, used in struct
>> tc_action_ops type field, from other macros starting with TCA_ACT_.
>> 
>> To make things clearer, we name the enum defining the TCA_ID_*
>> identifiers and also change the "type" field of struct tc_action to
>> id.
>> 
>> Signed-off-by: Eli Cohen 
>> Acked-by: Jiri Pirko 
>
>...
>
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 7ab55f97e7c4..51a0496f78ea 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -85,7 +85,7 @@ enum {
>>  #define TCA_ACT_SAMPLE 26
>>  
>>  /* Action type identifiers*/
>> -enum {
>> +enum tca_id {
>>  TCA_ID_UNSPEC = 0,
>>  TCA_ID_POLICE = 1,
>>  TCA_ID_GACT = TCA_ACT_GACT,
>
>This change updates the UAPI. It seems to me that it would not
>break existing users. But I would like to ask if this has been
>given due consideration.

Sure it has. I believe this is UAPI-safe change.

Re: [PATCH net-next 2/2] net: Change TCA_ACT_* to TCA_ID_* to match that of TCA_ID_POLICE

2019-02-08 Thread Simon Horman 
On Thu, Feb 07, 2019 at 09:45:49AM +0200, Eli Cohen wrote:
> Modify the kernel users of the TCA_ACT_* macros to use TCA_ID_*. For
> example, use TCA_ID_GACT instead of TCA_ACT_GACT. This will align with
> TCA_ID_POLICE and also differentiates these identifier, used in struct
> tc_action_ops type field, from other macros starting with TCA_ACT_.
> 
> To make things clearer, we name the enum defining the TCA_ID_*
> identifiers and also change the "type" field of struct tc_action to
> id.
> 
> Signed-off-by: Eli Cohen 
> Acked-by: Jiri Pirko 

...

> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 7ab55f97e7c4..51a0496f78ea 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -85,7 +85,7 @@ enum {
>  #define TCA_ACT_SAMPLE 26
>  
>  /* Action type identifiers*/
> -enum {
> +enum tca_id {
>   TCA_ID_UNSPEC = 0,
>   TCA_ID_POLICE = 1,
>   TCA_ID_GACT = TCA_ACT_GACT,

This change updates the UAPI. It seems to me that it would not
break existing users. But I would like to ask if this has been
given due consideration.

...

Re: [PATCH net-next 2/2] net: Change TCA_ACT_* to TCA_ID_* to match that of TCA_ID_POLICE

2019-02-07 Thread David Miller 
From: Eli Cohen 
Date: Thu,  7 Feb 2019 09:45:49 +0200

> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 902957beceb3..d54cb608dbaf 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -19,8 +19,6 @@
>  #include 
>  #include 
>  
> -#define TCA_ACT_SIMP 22
> -
>  #include 
>  #include 
>  

I would do this in patch #1.

Actually, because you didn't, after patch #1 there are two #defines
evaluated for this macro.  One in pkt_cls.h and one here.

The only reason the compiler doesn't warn and complain is because the
definitions are identical.

Thank you.

[PATCH net-next 2/2] net: Change TCA_ACT_* to TCA_ID_* to match that of TCA_ID_POLICE

2019-02-06 Thread Eli Cohen 
Modify the kernel users of the TCA_ACT_* macros to use TCA_ID_*. For
example, use TCA_ID_GACT instead of TCA_ACT_GACT. This will align with
TCA_ID_POLICE and also differentiates these identifier, used in struct
tc_action_ops type field, from other macros starting with TCA_ACT_.

To make things clearer, we name the enum defining the TCA_ID_*
identifiers and also change the "type" field of struct tc_action to
id.

Signed-off-by: Eli Cohen 
Acked-by: Jiri Pirko 
---
 include/net/act_api.h  | 2 +-
 include/net/tc_act/tc_csum.h   | 2 +-
 include/net/tc_act/tc_gact.h   | 2 +-
 include/net/tc_act/tc_mirred.h | 4 ++--
 include/net/tc_act/tc_pedit.h  | 2 +-
 include/net/tc_act/tc_sample.h | 2 +-
 include/net/tc_act/tc_skbedit.h| 2 +-
 include/net/tc_act/tc_tunnel_key.h | 4 ++--
 include/net/tc_act/tc_vlan.h   | 2 +-
 include/uapi/linux/pkt_cls.h   | 2 +-
 net/sched/act_api.c| 2 +-
 net/sched/act_bpf.c| 2 +-
 net/sched/act_connmark.c   | 2 +-
 net/sched/act_csum.c   | 2 +-
 net/sched/act_gact.c   | 2 +-
 net/sched/act_ife.c| 2 +-
 net/sched/act_ipt.c| 4 ++--
 net/sched/act_mirred.c | 2 +-
 net/sched/act_nat.c| 2 +-
 net/sched/act_pedit.c  | 2 +-
 net/sched/act_police.c | 2 +-
 net/sched/act_sample.c | 2 +-
 net/sched/act_simple.c | 4 +---
 net/sched/act_skbedit.c| 2 +-
 net/sched/act_skbmod.c | 2 +-
 net/sched/act_tunnel_key.c | 2 +-
 net/sched/act_vlan.c   | 2 +-
 27 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index dbc795ec659e..c745e9ccfab2 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -80,7 +80,7 @@ static inline void tcf_tm_dump(struct tcf_t *dtm, const 
struct tcf_t *stm)
 struct tc_action_ops {
struct list_head head;
charkind[IFNAMSIZ];
-   __u32   type; /* TBD to match kind */
+   enum tca_id  id; /* identifier should match kind */
size_t  size;
struct module   *owner;
int (*act)(struct sk_buff *, const struct tc_action *,
diff --git a/include/net/tc_act/tc_csum.h b/include/net/tc_act/tc_csum.h
index 32d2454c0479..68269e4581b7 100644
--- a/include/net/tc_act/tc_csum.h
+++ b/include/net/tc_act/tc_csum.h
@@ -21,7 +21,7 @@ struct tcf_csum {
 static inline bool is_tcf_csum(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-   if (a->ops && a->ops->type == TCA_ACT_CSUM)
+   if (a->ops && a->ops->id == TCA_ID_CSUM)
return true;
 #endif
return false;
diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
index ef8dd0db70ce..ee8d005f56fc 100644
--- a/include/net/tc_act/tc_gact.h
+++ b/include/net/tc_act/tc_gact.h
@@ -22,7 +22,7 @@ static inline bool __is_tcf_gact_act(const struct tc_action 
*a, int act,
 #ifdef CONFIG_NET_CLS_ACT
struct tcf_gact *gact;
 
-   if (a->ops && a->ops->type != TCA_ACT_GACT)
+   if (a->ops && a->ops->id != TCA_ID_GACT)
return false;
 
gact = to_gact(a);
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index a2e9cbca5c9e..c757585a05b0 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -17,7 +17,7 @@ struct tcf_mirred {
 static inline bool is_tcf_mirred_egress_redirect(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-   if (a->ops && a->ops->type == TCA_ACT_MIRRED)
+   if (a->ops && a->ops->id == TCA_ID_MIRRED)
return to_mirred(a)->tcfm_eaction == TCA_EGRESS_REDIR;
 #endif
return false;
@@ -26,7 +26,7 @@ static inline bool is_tcf_mirred_egress_redirect(const struct 
tc_action *a)
 static inline bool is_tcf_mirred_egress_mirror(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-   if (a->ops && a->ops->type == TCA_ACT_MIRRED)
+   if (a->ops && a->ops->id == TCA_ID_MIRRED)
return to_mirred(a)->tcfm_eaction == TCA_EGRESS_MIRROR;
 #endif
return false;
diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index fac3ad4a86de..748cf87a4d7e 100644
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -23,7 +23,7 @@ struct tcf_pedit {
 static inline bool is_tcf_pedit(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-   if (a->ops && a->ops->type == TCA_ACT_PEDIT)
+   if (a->ops && a->ops->id == TCA_ID_PEDIT)
return true;
 #endif
return false;
diff --git a/include/net/tc_act/tc_sample.h b/include/net/tc_act/tc_sample.h
index 01dbfea32672..0a559d4b6f0f 100644
--- a/include/net/tc_act/tc_sample.h
+++ b/include/net/tc_act/tc_sample.h
@@ -20,7 +20,7 @@ struct tcf_sample {
 static inline bool is_tcf_sample(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-   return a->ops &&