public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
@ 2016-07-02 13:26 Jamal Hadi Salim
  2016-07-02 13:49 ` Nikolay Aleksandrov
  0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2016-07-02 13:26 UTC (permalink / raw)
  To: davem; +Cc: netdev, xiyou.wangcong, daniel, Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

Often redirecting or mirroring requires that we set the MAC address
of the target device. While it is possible to pipe to a pedit action
this obsoletes the need for that. This is justified feature because
the dst MAC addresses rewrite is such a common use case.

Sample usage:
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15

This will match all icmp packets going out on dev $ETH and
redirect them to dev $SPANPORT while setting their dst MAC address
to 02:15:15:15:15:15

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/tc_act/tc_mirred.h        |  4 +++-
 include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
 net/sched/act_mirred.c                | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index e891835..7e8bced 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -6,10 +6,12 @@
 
 struct tcf_mirred {
 	struct tcf_common	common;
+	struct net_device __rcu	*tcfm_dev;
 	int			tcfm_eaction;
 	int			tcfm_ifindex;
 	int			tcfm_ok_push;
-	struct net_device __rcu	*tcfm_dev;
+	u8			eth_dst[ETH_ALEN];
+	/* XXX 6 bytes hole here*/
 	struct list_head	tcfm_list;
 };
 #define to_mirred(a) \
diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
index 3d7a2b3..aaca1ff 100644
--- a/include/uapi/linux/tc_act/tc_mirred.h
+++ b/include/uapi/linux/tc_act/tc_mirred.h
@@ -9,20 +9,21 @@
 #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
 #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
 #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
-                                                                                
+
 struct tc_mirred {
 	tc_gen;
 	int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
 	__u32                   ifindex;  /* ifindex of egress port */
 };
-                                                                                
+
 enum {
 	TCA_MIRRED_UNSPEC,
 	TCA_MIRRED_TM,
 	TCA_MIRRED_PARMS,
 	TCA_MIRRED_PAD,
+	TCA_MIRRED_DMAC,
 	__TCA_MIRRED_MAX
 };
 #define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1)
-                                                                                
+
 #endif
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 5b135d3..044648d 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -64,15 +64,21 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	struct net_device *dev;
 	int ret, ok_push = 0;
 	bool exists = false;
+	u8 *daddr = NULL;
 
 	if (nla == NULL)
 		return -EINVAL;
+
 	ret = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
 	if (ret < 0)
 		return ret;
+
 	if (tb[TCA_MIRRED_PARMS] == NULL)
 		return -EINVAL;
+
 	parm = nla_data(tb[TCA_MIRRED_PARMS]);
+	if (tb[TCA_MIRRED_DMAC])
+		daddr = nla_data(tb[TCA_MIRRED_DMAC]);
 
 	exists = tcf_hash_check(tn, parm->index, a, bind);
 	if (exists && bind)
@@ -126,6 +132,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	}
 	m = to_mirred(a);
 
+	if (daddr)
+		ether_addr_copy(m->eth_dst, daddr);
+
 	ASSERT_RTNL();
 	m->tcf_action = parm->action;
 	m->tcfm_eaction = parm->eaction;
@@ -190,6 +199,9 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 
 	skb2->skb_iif = skb->dev->ifindex;
 	skb2->dev = dev;
+	if (!is_zero_ether_addr(m->eth_dst))
+		ether_addr_copy(eth_hdr(skb2)->h_dest, m->eth_dst);
+
 	err = dev_queue_xmit(skb2);
 
 	if (err) {
@@ -203,7 +215,8 @@ out:
 	return retval;
 }
 
-static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
+			   int ref)
 {
 	unsigned char *b = skb_tail_pointer(skb);
 	struct tcf_mirred *m = a->priv;
@@ -220,6 +233,11 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, i
 	if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
 
+	if (!is_zero_ether_addr(m->eth_dst)) {
+		if (nla_put(skb, TCA_MIRRED_DMAC, ETH_ALEN, m->eth_dst))
+			goto nla_put_failure;
+	}
+
 	tcf_tm_dump(&t, &m->tcf_tm);
 	if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
 		goto nla_put_failure;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
  2016-07-02 13:26 [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address Jamal Hadi Salim
@ 2016-07-02 13:49 ` Nikolay Aleksandrov
  2016-07-02 14:02   ` Jamal Hadi Salim
  0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2016-07-02 13:49 UTC (permalink / raw)
  To: Jamal Hadi Salim, davem; +Cc: netdev, xiyou.wangcong, daniel

On 02/07/16 15:26, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Often redirecting or mirroring requires that we set the MAC address
> of the target device. While it is possible to pipe to a pedit action
> this obsoletes the need for that. This is justified feature because
> the dst MAC addresses rewrite is such a common use case.
> 
> Sample usage:
> sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
> u32 match ip protocol 1 0xff flowid 1:2 \
> action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
> 
> This will match all icmp packets going out on dev $ETH and
> redirect them to dev $SPANPORT while setting their dst MAC address
> to 02:15:15:15:15:15
> 
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  include/net/tc_act/tc_mirred.h        |  4 +++-
>  include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
>  net/sched/act_mirred.c                | 20 +++++++++++++++++++-
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
> index e891835..7e8bced 100644
> --- a/include/net/tc_act/tc_mirred.h
> +++ b/include/net/tc_act/tc_mirred.h
> @@ -6,10 +6,12 @@
>  
>  struct tcf_mirred {
>  	struct tcf_common	common;
> +	struct net_device __rcu	*tcfm_dev;
>  	int			tcfm_eaction;
>  	int			tcfm_ifindex;
>  	int			tcfm_ok_push;
> -	struct net_device __rcu	*tcfm_dev;
> +	u8			eth_dst[ETH_ALEN];
> +	/* XXX 6 bytes hole here*/
>  	struct list_head	tcfm_list;
>  };
>  #define to_mirred(a) \
> diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
> index 3d7a2b3..aaca1ff 100644
> --- a/include/uapi/linux/tc_act/tc_mirred.h
> +++ b/include/uapi/linux/tc_act/tc_mirred.h
> @@ -9,20 +9,21 @@
>  #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
>  #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
>  #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
> -                                                                                
> +
>  struct tc_mirred {
>  	tc_gen;
>  	int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
>  	__u32                   ifindex;  /* ifindex of egress port */
>  };
> -                                                                                
> +
>  enum {
>  	TCA_MIRRED_UNSPEC,
>  	TCA_MIRRED_TM,
>  	TCA_MIRRED_PARMS,
>  	TCA_MIRRED_PAD,
> +	TCA_MIRRED_DMAC,

Hi Jamal,
I think you should update "mirred_policy" in order to ensure that the attribute has
the minimum length for a mac address. Also a minor suggestion - maybe err out on a
zero mac address, otherwise the user might think the operation was successful.

Cheers,
 Nik

>  	__TCA_MIRRED_MAX
>  };
>  #define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1)
> -                                                                                
> +
>  #endif
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 5b135d3..044648d 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -64,15 +64,21 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>  	struct net_device *dev;
>  	int ret, ok_push = 0;
>  	bool exists = false;
> +	u8 *daddr = NULL;
>  
>  	if (nla == NULL)
>  		return -EINVAL;
> +
>  	ret = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
>  	if (ret < 0)
>  		return ret;
> +
>  	if (tb[TCA_MIRRED_PARMS] == NULL)
>  		return -EINVAL;
> +
>  	parm = nla_data(tb[TCA_MIRRED_PARMS]);
> +	if (tb[TCA_MIRRED_DMAC])
> +		daddr = nla_data(tb[TCA_MIRRED_DMAC]);
>  
>  	exists = tcf_hash_check(tn, parm->index, a, bind);
>  	if (exists && bind)
> @@ -126,6 +132,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>  	}
>  	m = to_mirred(a);
>  
> +	if (daddr)
> +		ether_addr_copy(m->eth_dst, daddr);
> +
>  	ASSERT_RTNL();
>  	m->tcf_action = parm->action;
>  	m->tcfm_eaction = parm->eaction;
> @@ -190,6 +199,9 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
>  
>  	skb2->skb_iif = skb->dev->ifindex;
>  	skb2->dev = dev;
> +	if (!is_zero_ether_addr(m->eth_dst))
> +		ether_addr_copy(eth_hdr(skb2)->h_dest, m->eth_dst);
> +
>  	err = dev_queue_xmit(skb2);
>  
>  	if (err) {
> @@ -203,7 +215,8 @@ out:
>  	return retval;
>  }
>  
> -static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
> +static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
> +			   int ref)
>  {
>  	unsigned char *b = skb_tail_pointer(skb);
>  	struct tcf_mirred *m = a->priv;
> @@ -220,6 +233,11 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, i
>  	if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
>  		goto nla_put_failure;
>  
> +	if (!is_zero_ether_addr(m->eth_dst)) {
> +		if (nla_put(skb, TCA_MIRRED_DMAC, ETH_ALEN, m->eth_dst))
> +			goto nla_put_failure;
> +	}
> +
>  	tcf_tm_dump(&t, &m->tcf_tm);
>  	if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
>  		goto nla_put_failure;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
  2016-07-02 13:49 ` Nikolay Aleksandrov
@ 2016-07-02 14:02   ` Jamal Hadi Salim
  2016-07-02 14:07     ` Nikolay Aleksandrov
  0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2016-07-02 14:02 UTC (permalink / raw)
  To: Nikolay Aleksandrov, davem; +Cc: netdev, xiyou.wangcong, daniel

On 16-07-02 09:49 AM, Nikolay Aleksandrov wrote:
> On 02/07/16 15:26, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Often redirecting or mirroring requires that we set the MAC address
>> of the target device. While it is possible to pipe to a pedit action
>> this obsoletes the need for that. This is justified feature because
>> the dst MAC addresses rewrite is such a common use case.
>>
>> Sample usage:
>> sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
>> u32 match ip protocol 1 0xff flowid 1:2 \
>> action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
>>
>> This will match all icmp packets going out on dev $ETH and
>> redirect them to dev $SPANPORT while setting their dst MAC address
>> to 02:15:15:15:15:15
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>>   include/net/tc_act/tc_mirred.h        |  4 +++-
>>   include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
>>   net/sched/act_mirred.c                | 20 +++++++++++++++++++-
>>   3 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>> index e891835..7e8bced 100644
>> --- a/include/net/tc_act/tc_mirred.h
>> +++ b/include/net/tc_act/tc_mirred.h
>> @@ -6,10 +6,12 @@
>>
>>   struct tcf_mirred {
>>   	struct tcf_common	common;
>> +	struct net_device __rcu	*tcfm_dev;
>>   	int			tcfm_eaction;
>>   	int			tcfm_ifindex;
>>   	int			tcfm_ok_push;
>> -	struct net_device __rcu	*tcfm_dev;
>> +	u8			eth_dst[ETH_ALEN];
>> +	/* XXX 6 bytes hole here*/
>>   	struct list_head	tcfm_list;
>>   };
>>   #define to_mirred(a) \
>> diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
>> index 3d7a2b3..aaca1ff 100644
>> --- a/include/uapi/linux/tc_act/tc_mirred.h
>> +++ b/include/uapi/linux/tc_act/tc_mirred.h
>> @@ -9,20 +9,21 @@
>>   #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
>>   #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
>>   #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
>> -
>> +
>>   struct tc_mirred {
>>   	tc_gen;
>>   	int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
>>   	__u32                   ifindex;  /* ifindex of egress port */
>>   };
>> -
>> +
>>   enum {
>>   	TCA_MIRRED_UNSPEC,
>>   	TCA_MIRRED_TM,
>>   	TCA_MIRRED_PARMS,
>>   	TCA_MIRRED_PAD,
>> +	TCA_MIRRED_DMAC,
>
> Hi Jamal,
> I think you should update "mirred_policy" in order to ensure that the attribute has
> the minimum length for a mac address.

Good point. Will do in the next update.

> Also a minor suggestion - maybe err out on a
> zero mac address, otherwise the user might think the operation was successful.
>

Is a zero mac address wrong? What if that was policy intent?

cheers,
jamal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
  2016-07-02 14:02   ` Jamal Hadi Salim
@ 2016-07-02 14:07     ` Nikolay Aleksandrov
  2016-07-02 14:16       ` Jamal Hadi Salim
  0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2016-07-02 14:07 UTC (permalink / raw)
  To: Jamal Hadi Salim, davem; +Cc: netdev, xiyou.wangcong, daniel

On 02/07/16 16:02, Jamal Hadi Salim wrote:
> On 16-07-02 09:49 AM, Nikolay Aleksandrov wrote:
>> On 02/07/16 15:26, Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>
>>> Often redirecting or mirroring requires that we set the MAC address
>>> of the target device. While it is possible to pipe to a pedit action
>>> this obsoletes the need for that. This is justified feature because
>>> the dst MAC addresses rewrite is such a common use case.
>>>
>>> Sample usage:
>>> sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
>>> u32 match ip protocol 1 0xff flowid 1:2 \
>>> action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
>>>
>>> This will match all icmp packets going out on dev $ETH and
>>> redirect them to dev $SPANPORT while setting their dst MAC address
>>> to 02:15:15:15:15:15
>>>
>>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>> ---
>>>   include/net/tc_act/tc_mirred.h        |  4 +++-
>>>   include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
>>>   net/sched/act_mirred.c                | 20 +++++++++++++++++++-
>>>   3 files changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>>> index e891835..7e8bced 100644
>>> --- a/include/net/tc_act/tc_mirred.h
>>> +++ b/include/net/tc_act/tc_mirred.h
>>> @@ -6,10 +6,12 @@
>>>
>>>   struct tcf_mirred {
>>>       struct tcf_common    common;
>>> +    struct net_device __rcu    *tcfm_dev;
>>>       int            tcfm_eaction;
>>>       int            tcfm_ifindex;
>>>       int            tcfm_ok_push;
>>> -    struct net_device __rcu    *tcfm_dev;
>>> +    u8            eth_dst[ETH_ALEN];
>>> +    /* XXX 6 bytes hole here*/
>>>       struct list_head    tcfm_list;
>>>   };
>>>   #define to_mirred(a) \
>>> diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
>>> index 3d7a2b3..aaca1ff 100644
>>> --- a/include/uapi/linux/tc_act/tc_mirred.h
>>> +++ b/include/uapi/linux/tc_act/tc_mirred.h
>>> @@ -9,20 +9,21 @@
>>>   #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
>>>   #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
>>>   #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
>>> -
>>> +
>>>   struct tc_mirred {
>>>       tc_gen;
>>>       int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
>>>       __u32                   ifindex;  /* ifindex of egress port */
>>>   };
>>> -
>>> +
>>>   enum {
>>>       TCA_MIRRED_UNSPEC,
>>>       TCA_MIRRED_TM,
>>>       TCA_MIRRED_PARMS,
>>>       TCA_MIRRED_PAD,
>>> +    TCA_MIRRED_DMAC,
>>
>> Hi Jamal,
>> I think you should update "mirred_policy" in order to ensure that the attribute has
>> the minimum length for a mac address.
> 
> Good point. Will do in the next update.
> 
>> Also a minor suggestion - maybe err out on a
>> zero mac address, otherwise the user might think the operation was successful.
>>
> 
> Is a zero mac address wrong? What if that was policy intent?
> 

If you mean that you give the user ability to get rid of the mac, then okay. I said it
because it will seem like a successful operation and then the mac will not be dumped
or overwritten which will look like it wasn't set at all.

> cheers,
> jamal
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
  2016-07-02 14:07     ` Nikolay Aleksandrov
@ 2016-07-02 14:16       ` Jamal Hadi Salim
  2016-07-02 14:26         ` Jamal Hadi Salim
  0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2016-07-02 14:16 UTC (permalink / raw)
  To: Nikolay Aleksandrov, davem; +Cc: netdev, xiyou.wangcong, daniel

On 16-07-02 10:07 AM, Nikolay Aleksandrov wrote:
> On 02/07/16 16:02, Jamal Hadi Salim wrote:
>> On 16-07-02 09:49 AM, Nikolay Aleksandrov wrote:

>>> Also a minor suggestion - maybe err out on a
>>> zero mac address, otherwise the user might think the operation was successful.
>>>
>>
>> Is a zero mac address wrong? What if that was policy intent?
>>
>
> If you mean that you give the user ability to get rid of the mac, then okay. I said it
> because it will seem like a successful operation and then the mac will not be dumped
> or overwritten which will look like it wasn't set at all.
>

I mean a MAC address of all zeros.
The destination MAC itself is optional attribute; if user doesnt pass it
we (dont care and) never use it. Dumping will not see it and therefore
not display it.

cheers,
jamal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address
  2016-07-02 14:16       ` Jamal Hadi Salim
@ 2016-07-02 14:26         ` Jamal Hadi Salim
  0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2016-07-02 14:26 UTC (permalink / raw)
  To: Nikolay Aleksandrov, davem; +Cc: netdev, xiyou.wangcong, daniel

On 16-07-02 10:16 AM, Jamal Hadi Salim wrote:
> On 16-07-02 10:07 AM, Nikolay Aleksandrov wrote:
>> On 02/07/16 16:02, Jamal Hadi Salim wrote:
>>> On 16-07-02 09:49 AM, Nikolay Aleksandrov wrote:
>
>>>> Also a minor suggestion - maybe err out on a
>>>> zero mac address, otherwise the user might think the operation was
>>>> successful.
>>>>
>>>
>>> Is a zero mac address wrong? What if that was policy intent?
>>>
>>
>> If you mean that you give the user ability to get rid of the mac, then
>> okay. I said it
>> because it will seem like a successful operation and then the mac will
>> not be dumped
>> or overwritten which will look like it wasn't set at all.
>>
>
> I mean a MAC address of all zeros.
> The destination MAC itself is optional attribute; if user doesnt pass it
> we (dont care and) never use it. Dumping will not see it and therefore
> not display it.
>

Actually it makes sense not to allow a MAC address of all zeros.
So I will add that check. Posting v2 shortly.

cheers,
jamal

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-07-02 14:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-02 13:26 [PATCH net-next 1/1] net sched actions: mirred add support for setting Dst MAC address Jamal Hadi Salim
2016-07-02 13:49 ` Nikolay Aleksandrov
2016-07-02 14:02   ` Jamal Hadi Salim
2016-07-02 14:07     ` Nikolay Aleksandrov
2016-07-02 14:16       ` Jamal Hadi Salim
2016-07-02 14:26         ` Jamal Hadi Salim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox