On Mon, Oct 26, 2020 at 09:48:24AM +0100, Simon Horman wrote:
> On Sun, Oct 25, 2020 at 03:18:48PM -0600, David Ahern wrote:
> > On 10/19/20 5:47 AM, Zahari Doychev wrote:
> > > Currently the icmp and arp prsing functions are called with inccorect
> > > ethtype in case of vlan or cvlan filter options. In this case either
> > > cvlan_ethtype or vlan_ethtype has to be used.
> > >
> > > Signed-off-by: Zahari Doychev <zahari.doyc...@linux.com>
> > > ---
> > > tc/f_flower.c | 43 ++++++++++++++++++++++++++-----------------
> > > 1 file changed, 26 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/tc/f_flower.c b/tc/f_flower.c
> > > index 00c919fd..dd9f3446 100644
> > > --- a/tc/f_flower.c
> > > +++ b/tc/f_flower.c
> > > @@ -1712,7 +1712,10 @@ static int flower_parse_opt(struct filter_util
> > > *qu, char *handle,
> > > }
> > > } else if (matches(*argv, "type") == 0) {
> > > NEXT_ARG();
> > > - ret = flower_parse_icmp(*argv, eth_type, ip_proto,
> > > + ret = flower_parse_icmp(*argv, cvlan_ethtype ?
> > > + cvlan_ethtype : vlan_ethtype ?
> > > + vlan_ethtype : eth_type,
> > > + ip_proto,
> >
> > looks correct to me, but would like confirmation of the intent from Simon.
>
> Thanks, this appears to be correct to me as ultimately
> the code wants to operate on ETH_P_IP or ETH_P_IPV6 rather
> than a VLAN Ether type.
>
> > Also, I am not a fan of the readability of that coding style. Rather
> > than repeat that expression multiple times, make a short helper to
> > return the relevant eth type and use a temp variable for it. You should
> > also comment that relevant eth type changes as arguments are parsed.
I will add the helper and resend.
Thanks Zahari
> >
> > Thanks,
> >
> >
