diff mbox series

[iproute2-next] tc flower: use right ethertype in icmp/arp parsing

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

Commit Message

Zahari Doychev Oct. 19, 2020, 11:47 a.m. UTC
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(-)

Comments

David Ahern Oct. 25, 2020, 9:18 p.m. UTC | #1
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,
Simon Horman Oct. 26, 2020, 8:48 a.m. UTC | #2
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,
> 
>
Zahari Doychev Oct. 27, 2020, 9:11 a.m. UTC | #3
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,
> > 
> >
David Ahern Oct. 27, 2020, 2:12 p.m. UTC | #4
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 mbox series

Patch

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;