[PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

Kernel expects type RTN_UNICAST for mpls route/dels

Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 ip/iproute.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ip/iproute.c b/ip/iproute.c
index 670a4c6..71c088b 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
 	int scope_ok = 0;
 	int table_ok = 0;
 	int raw = 0;
+	int type_ok = 0;
 
 	memset(&req, 0, sizeof(req));
 
@@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
 			    rtnl_rtntype_a2n(&type, *argv) == 0) {
 				NEXT_ARG();
 				req.r.rtm_type = type;
+				type_ok = 1;
 			}
 
 			if (matches(*argv, "help") == 0)
@@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
 		}
 	}
 
+	if (!type_ok && req.r.rtm_family == AF_MPLS)
+		req.r.rtm_type = RTN_UNICAST;
+
 	if (req.r.rtm_family == AF_UNSPEC)
 		req.r.rtm_family = AF_INET;
 
-- 
1.7.10.4

Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[Robert Shearman]

On 27/05/15 19:37, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Kernel expects type RTN_UNICAST for mpls route/dels
>
> Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>   ip/iproute.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 670a4c6..71c088b 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   	int scope_ok = 0;
>   	int table_ok = 0;
>   	int raw = 0;
> +	int type_ok = 0;
>
>   	memset(&req, 0, sizeof(req));
>
> @@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   			    rtnl_rtntype_a2n(&type, *argv) == 0) {
>   				NEXT_ARG();
>   				req.r.rtm_type = type;
> +				type_ok = 1;
>   			}
>
>   			if (matches(*argv, "help") == 0)
> @@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   		}
>   	}
>
> +	if (!type_ok && req.r.rtm_family == AF_MPLS)
> +		req.r.rtm_type = RTN_UNICAST;
> +
>   	if (req.r.rtm_family == AF_UNSPEC)
>   		req.r.rtm_family = AF_INET;
>
>

There is this block of code near the start of iproute_modify that sets 
req.r.rtm_type in the add/modify cases:

         if (cmd != RTM_DELROUTE) {
                 req.r.rtm_protocol = RTPROT_BOOT;
                 req.r.rtm_scope = RT_SCOPE_UNIVERSE;
                 req.r.rtm_type = RTN_UNICAST;
         }

How about doing similar for the mpls delete case? This would avoid the 
need to track if the type has been set and would also make the way 
rtm_type is set in the delete case as close as possible to that in the 
add/modify cases.

Thanks,
Rob

Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[roopa]

On 5/27/15, 12:59 PM, Robert Shearman wrote:
> On 27/05/15 19:37, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Kernel expects type RTN_UNICAST for mpls route/dels
>>
>> Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>>   ip/iproute.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/ip/iproute.c b/ip/iproute.c
>> index 670a4c6..71c088b 100644
>> --- a/ip/iproute.c
>> +++ b/ip/iproute.c
>> @@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned 
>> flags, int argc, char **argv)
>>       int scope_ok = 0;
>>       int table_ok = 0;
>>       int raw = 0;
>> +    int type_ok = 0;
>>
>>       memset(&req, 0, sizeof(req));
>>
>> @@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned 
>> flags, int argc, char **argv)
>>                   rtnl_rtntype_a2n(&type, *argv) == 0) {
>>                   NEXT_ARG();
>>                   req.r.rtm_type = type;
>> +                type_ok = 1;
>>               }
>>
>>               if (matches(*argv, "help") == 0)
>> @@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned 
>> flags, int argc, char **argv)
>>           }
>>       }
>>
>> +    if (!type_ok && req.r.rtm_family == AF_MPLS)
>> +        req.r.rtm_type = RTN_UNICAST;
>> +
>>       if (req.r.rtm_family == AF_UNSPEC)
>>           req.r.rtm_family = AF_INET;
>>
>>
>
> There is this block of code near the start of iproute_modify that sets 
> req.r.rtm_type in the add/modify cases:
>
>         if (cmd != RTM_DELROUTE) {
>                 req.r.rtm_protocol = RTPROT_BOOT;
>                 req.r.rtm_scope = RT_SCOPE_UNIVERSE;
>                 req.r.rtm_type = RTN_UNICAST;
>         }
>
> How about doing similar for the mpls delete case? This would avoid the 
> need to track if the type has been set and would also make the way 
> rtm_type is set in the delete case as close as possible to that in the 
> add/modify cases.
sure that works too. There was already *_ok checks for the rest of the 
attributes, ..so added it there.

v3 ...coming...

Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[roopa]

On 5/27/15, 1:08 PM, roopa wrote:
> On 5/27/15, 12:59 PM, Robert Shearman wrote:
>> On 27/05/15 19:37, Roopa Prabhu wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> Kernel expects type RTN_UNICAST for mpls route/dels
>>>
>>> Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>> ---
>>>   ip/iproute.c |    5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/ip/iproute.c b/ip/iproute.c
>>> index 670a4c6..71c088b 100644
>>> --- a/ip/iproute.c
>>> +++ b/ip/iproute.c
>>> @@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned 
>>> flags, int argc, char **argv)
>>>       int scope_ok = 0;
>>>       int table_ok = 0;
>>>       int raw = 0;
>>> +    int type_ok = 0;
>>>
>>>       memset(&req, 0, sizeof(req));
>>>
>>> @@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned 
>>> flags, int argc, char **argv)
>>>                   rtnl_rtntype_a2n(&type, *argv) == 0) {
>>>                   NEXT_ARG();
>>>                   req.r.rtm_type = type;
>>> +                type_ok = 1;
>>>               }
>>>
>>>               if (matches(*argv, "help") == 0)
>>> @@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned 
>>> flags, int argc, char **argv)
>>>           }
>>>       }
>>>
>>> +    if (!type_ok && req.r.rtm_family == AF_MPLS)
>>> +        req.r.rtm_type = RTN_UNICAST;
>>> +
>>>       if (req.r.rtm_family == AF_UNSPEC)
>>>           req.r.rtm_family = AF_INET;
>>>
>>>
>>
>> There is this block of code near the start of iproute_modify that 
>> sets req.r.rtm_type in the add/modify cases:
>>
>>         if (cmd != RTM_DELROUTE) {
>>                 req.r.rtm_protocol = RTPROT_BOOT;
>>                 req.r.rtm_scope = RT_SCOPE_UNIVERSE;
>>                 req.r.rtm_type = RTN_UNICAST;
>>         }
>>
>> How about doing similar for the mpls delete case? This would avoid 
>> the need to track if the type has been set and would also make the 
>> way rtm_type is set in the delete case as close as possible to that 
>> in the add/modify cases.
> sure that works too. There was already *_ok checks for the rest of the 
> attributes, ..so added it there.
>
> v3 ...coming...
>
looking at the code again..now i remember why i have it this way. I will 
have to add a check for family around
the code you point out above. And it some cases if the user has not 
specified the family explicitly, we derive the msg family
in the while loop that parses the args...based on the other arguments 
given by the user.
In the particular mpls case though, user explicitly specifies the family 
and moving the patch to the code you point above should be ok.

But to be consistent with the rest of the code, it seems better to do 
the check and set the defaults at the end after parsing all the args.

So, now i am inclined to keep the v2 patch as is...unless you have 
strong reasons.

thanks.

Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[Robert Shearman]

On 28/05/15 01:06, roopa wrote:
> On 5/27/15, 1:08 PM, roopa wrote:
>> On 5/27/15, 12:59 PM, Robert Shearman wrote:
>>> On 27/05/15 19:37, Roopa Prabhu wrote:
>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>
>>>> Kernel expects type RTN_UNICAST for mpls route/dels
>>>>
>>>> Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>> ---
>>>>   ip/iproute.c |    5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/ip/iproute.c b/ip/iproute.c
>>>> index 670a4c6..71c088b 100644
>>>> --- a/ip/iproute.c
>>>> +++ b/ip/iproute.c
>>>> @@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned
>>>> flags, int argc, char **argv)
>>>>       int scope_ok = 0;
>>>>       int table_ok = 0;
>>>>       int raw = 0;
>>>> +    int type_ok = 0;
>>>>
>>>>       memset(&req, 0, sizeof(req));
>>>>
>>>> @@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned
>>>> flags, int argc, char **argv)
>>>>                   rtnl_rtntype_a2n(&type, *argv) == 0) {
>>>>                   NEXT_ARG();
>>>>                   req.r.rtm_type = type;
>>>> +                type_ok = 1;
>>>>               }
>>>>
>>>>               if (matches(*argv, "help") == 0)
>>>> @@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned
>>>> flags, int argc, char **argv)
>>>>           }
>>>>       }
>>>>
>>>> +    if (!type_ok && req.r.rtm_family == AF_MPLS)
>>>> +        req.r.rtm_type = RTN_UNICAST;
>>>> +
>>>>       if (req.r.rtm_family == AF_UNSPEC)
>>>>           req.r.rtm_family = AF_INET;
>>>>
>>>>
>>>
>>> There is this block of code near the start of iproute_modify that
>>> sets req.r.rtm_type in the add/modify cases:
>>>
>>>         if (cmd != RTM_DELROUTE) {
>>>                 req.r.rtm_protocol = RTPROT_BOOT;
>>>                 req.r.rtm_scope = RT_SCOPE_UNIVERSE;
>>>                 req.r.rtm_type = RTN_UNICAST;
>>>         }
>>>
>>> How about doing similar for the mpls delete case? This would avoid
>>> the need to track if the type has been set and would also make the
>>> way rtm_type is set in the delete case as close as possible to that
>>> in the add/modify cases.
>> sure that works too. There was already *_ok checks for the rest of the
>> attributes, ..so added it there.
>>
>> v3 ...coming...
>>
> looking at the code again..now i remember why i have it this way. I will
> have to add a check for family around
> the code you point out above. And it some cases if the user has not
> specified the family explicitly, we derive the msg family
> in the while loop that parses the args...based on the other arguments
> given by the user.
> In the particular mpls case though, user explicitly specifies the family
> and moving the patch to the code you point above should be ok.
>
> But to be consistent with the rest of the code, it seems better to do
> the check and set the defaults at the end after parsing all the args.
>
> So, now i am inclined to keep the v2 patch as is...unless you have
> strong reasons.

Ah, yes, of course.

In that case, LGTM.

Thanks,
Rob

Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes

[Robert Shearman]

On 27/05/15 19:37, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Kernel expects type RTN_UNICAST for mpls route/dels
>
> Signed-off-by: Vivek Venkataraman <vivek@cumulusnetworks.com>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Reviewed-by: Robert Shearman <rshearma@brocade.com>

> ---
>   ip/iproute.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 670a4c6..71c088b 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -803,6 +803,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   	int scope_ok = 0;
>   	int table_ok = 0;
>   	int raw = 0;
> +	int type_ok = 0;
>
>   	memset(&req, 0, sizeof(req));
>
> @@ -1095,6 +1096,7 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   			    rtnl_rtntype_a2n(&type, *argv) == 0) {
>   				NEXT_ARG();
>   				req.r.rtm_type = type;
> +				type_ok = 1;
>   			}
>
>   			if (matches(*argv, "help") == 0)
> @@ -1160,6 +1162,9 @@ static int iproute_modify(int cmd, unsigned flags, int argc, char **argv)
>   		}
>   	}
>
> +	if (!type_ok && req.r.rtm_family == AF_MPLS)
> +		req.r.rtm_type = RTN_UNICAST;
> +
>   	if (req.r.rtm_family == AF_UNSPEC)
>   		req.r.rtm_family = AF_INET;
>
>