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