* [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes @ 2015-05-27 18:37 Roopa Prabhu 2015-05-27 19:59 ` Robert Shearman 2015-05-28 9:17 ` Robert Shearman 0 siblings, 2 replies; 6+ messages in thread From: Roopa Prabhu @ 2015-05-27 18:37 UTC (permalink / raw) To: ebiederm; +Cc: davem, rshearma, netdev, vivek 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes 2015-05-27 18:37 [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes Roopa Prabhu @ 2015-05-27 19:59 ` Robert Shearman 2015-05-27 20:08 ` roopa 2015-05-28 9:17 ` Robert Shearman 1 sibling, 1 reply; 6+ messages in thread From: Robert Shearman @ 2015-05-27 19:59 UTC (permalink / raw) To: Roopa Prabhu, ebiederm; +Cc: davem, netdev, vivek 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes 2015-05-27 19:59 ` Robert Shearman @ 2015-05-27 20:08 ` roopa 2015-05-28 0:06 ` roopa 0 siblings, 1 reply; 6+ messages in thread From: roopa @ 2015-05-27 20:08 UTC (permalink / raw) To: Robert Shearman; +Cc: ebiederm, davem, netdev, vivek 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... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes 2015-05-27 20:08 ` roopa @ 2015-05-28 0:06 ` roopa 2015-05-28 9:15 ` Robert Shearman 0 siblings, 1 reply; 6+ messages in thread From: roopa @ 2015-05-28 0:06 UTC (permalink / raw) To: Robert Shearman; +Cc: ebiederm, davem, netdev, vivek 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes 2015-05-28 0:06 ` roopa @ 2015-05-28 9:15 ` Robert Shearman 0 siblings, 0 replies; 6+ messages in thread From: Robert Shearman @ 2015-05-28 9:15 UTC (permalink / raw) To: roopa; +Cc: ebiederm, davem, netdev, vivek 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes 2015-05-27 18:37 [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes Roopa Prabhu 2015-05-27 19:59 ` Robert Shearman @ 2015-05-28 9:17 ` Robert Shearman 1 sibling, 0 replies; 6+ messages in thread From: Robert Shearman @ 2015-05-28 9:17 UTC (permalink / raw) To: Roopa Prabhu, ebiederm; +Cc: davem, netdev, vivek 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; > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-28 9:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-27 18:37 [PATCH net iproute2 v2 1/2] mpls: always set type as RTN_UNICAST for route add/deletes Roopa Prabhu 2015-05-27 19:59 ` Robert Shearman 2015-05-27 20:08 ` roopa 2015-05-28 0:06 ` roopa 2015-05-28 9:15 ` Robert Shearman 2015-05-28 9:17 ` Robert Shearman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).