| Message ID | 20201019114708.1050421-1-zahari.doychev@linux.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [iproute2-next] tc flower: use right ethertype in icmp/arp parsing | expand |
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.doychev@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. 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. Thanks,
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.doychev@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. > > Thanks, > >
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.doychev@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, > > > >
On 10/27/20 3:11 AM, Zahari Doychev wrote: > 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.doychev@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. > perhaps it is simpler to have a new local variable that tracks the eth type to be used in these locations.
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, FLOWER_ICMP_FIELD_TYPE, n); if (ret < 0) { fprintf(stderr, "Illegal \"icmp type\"\n"); @@ -1720,7 +1723,10 @@ static int flower_parse_opt(struct filter_util *qu, char *handle, } } else if (matches(*argv, "code") == 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, FLOWER_ICMP_FIELD_CODE, n); if (ret < 0) { fprintf(stderr, "Illegal \"icmp code\"\n"); @@ -1728,33 +1734,36 @@ static int flower_parse_opt(struct filter_util *qu, char *handle, } } else if (matches(*argv, "arp_tip") == 0) { NEXT_ARG(); - ret = flower_parse_arp_ip_addr(*argv, vlan_ethtype ? - vlan_ethtype : eth_type, - TCA_FLOWER_KEY_ARP_TIP, - TCA_FLOWER_KEY_ARP_TIP_MASK, - n); + ret = flower_parse_arp_ip_addr(*argv, cvlan_ethtype ? + cvlan_ethtype : vlan_ethtype ? + vlan_ethtype : eth_type, + TCA_FLOWER_KEY_ARP_TIP, + TCA_FLOWER_KEY_ARP_TIP_MASK, + n); if (ret < 0) { fprintf(stderr, "Illegal \"arp_tip\"\n"); return -1; } } else if (matches(*argv, "arp_sip") == 0) { NEXT_ARG(); - ret = flower_parse_arp_ip_addr(*argv, vlan_ethtype ? - vlan_ethtype : eth_type, - TCA_FLOWER_KEY_ARP_SIP, - TCA_FLOWER_KEY_ARP_SIP_MASK, - n); + ret = flower_parse_arp_ip_addr(*argv, cvlan_ethtype ? + cvlan_ethtype : vlan_ethtype ? + vlan_ethtype : eth_type, + TCA_FLOWER_KEY_ARP_SIP, + TCA_FLOWER_KEY_ARP_SIP_MASK, + n); if (ret < 0) { fprintf(stderr, "Illegal \"arp_sip\"\n"); return -1; } } else if (matches(*argv, "arp_op") == 0) { NEXT_ARG(); - ret = flower_parse_arp_op(*argv, vlan_ethtype ? - vlan_ethtype : eth_type, - TCA_FLOWER_KEY_ARP_OP, - TCA_FLOWER_KEY_ARP_OP_MASK, - n); + ret = flower_parse_arp_op(*argv, cvlan_ethtype ? + cvlan_ethtype : vlan_ethtype ? + vlan_ethtype : eth_type, + TCA_FLOWER_KEY_ARP_OP, + TCA_FLOWER_KEY_ARP_OP_MASK, + n); if (ret < 0) { fprintf(stderr, "Illegal \"arp_op\"\n"); return -1;
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.doychev@linux.com> --- tc/f_flower.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)