Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

[Boris Sukholitko <boris.sukholitko@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

On Thu, May 27, 2021 at 07:00:52PM +0200, Davide Caratti wrote:
[...]
> 
> My suggestion was just to simplify the end-user dump experience, so
> that the value of 'p->tcfv_push_prio' is dumped always in case of
> TCA_VLAN_ACT_PUSH. In this way, rules with equal "behavior" in the
> traffic path are always dumped in the same way. IOW,
> 
> # tc action add action vlan push id 42 prio 0 index 1
> 
> and
> 
> # tc action add action vlan push id 42 index 1
> 
> do exactly the same thing in the traffic path, so there is no need to
> dump them differently. On the contrary, these 2 rules:
> 
> # tc action add action vlan modify id 42 prio 0 index 1
> 
> and
> 
> # tc action add action vlan modify id 42 index 1
> 
> don't do the same thing, because packet hitting the first rule will have
> their priority identically set to 0, while the second one will leave the
> VLAN priority unmodified. So, I think it makes sense to have different
> dumps here (that was my comment to your v1).

I am convinced. I've done this in v3.

> 
> Another small nit - forgive me, I didn't spot it in the first review:
> 
> not 100% sure, but I think that in tcf_vlan_get_fill_size() we need
> to avoid accounting for TCA_VLAN_PUSH_VLAN_PRIORITY in case the rule
> has 'push_prio_exists' equal to false. Otherwise we allocate an
> extra u8 netlink attribute in case of batch dump. Correct?

Also done in v3.

Thanks,
Boris.

> 
> thanks!
> -- 
> davide
> 
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]