Bgpafisafi #107

donaldsharp · 2017-01-24T03:37:07Z

No description provided.

Allow the specification of a VRF_ALL to be used for CLI. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

The show_adj_route_vpn function is only currently used in conjunction with the KEEP_OLD_VPN_COMMANDS #define. Add this function to that define for the moment. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

The bgp_parse_safi function is never called remove it. Especially as that later commits will properly handle what this function was trying to do. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

The parser was incorrect for the 'set ... vpn nexthop ...' commands. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Create bgp_vty_find_and_parse_afi_safi_vrf that will parse the `show [ip] bgp [<view|vrf> WORD] [<ipv4|ipv6> [<unicast|multicast|vpn|encap>]]' part of a command and to return the correct spot we are in the command. Cleanup 'dampening parameters' part of this command. Consolidate the creation of the bgp data structure to be a bit cleaner. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Cleanup the bgp bestpath or multipath show commands. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Fix this command to use the correct format for a show command. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

1) Make [<view|vrf> WORD] consistent 2) Fix inconsistent help string 3) Fix the show .. vrf all command Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

NetDEF-CI · 2017-01-24T04:39:28Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-113/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

NetDEF-CI · 2017-01-24T15:47:17Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-115/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

louberger

Why:
+#define VRF_ALL UINT16_MAX - 1

pguibert6WIND · 2017-01-24T15:30:30Z

bgpd/bgp_vty.c

+    }
+
+  if (*vrf == VRF_UNKNOWN)
+    {


how do you initialise vrf in the case vrf_name is already null ( commands with no vrf at all)?
I would lie the *vrf value to be set inside that function.

I do not believe this is the right thing to do. Only the calling function knows what the right defaults are for the afi/safi/vrf due to we assume different defaults based upon the command. This approach would break the ability for the calling function to do the right thing. As I stated I will update the Pull Request with better documentation for this funciton.

pguibert6WIND · 2017-01-24T15:31:17Z

bgpd/bgp_route.c

+        }
+    }
+  else
+    bgp = NULL;



what happens in the case the command i use is a command without vrf either view parameter.

then the default value specified at the top of the function is used for the lookup.

It look like the change to pass in bgp to bgp_show_route can result in NULL deference. See line 9039. Suggest that bgp_show_route handle case of NULL bgp pointer (by getting default instance if NULL)

shouldn't line 8078 result in an error message and return, and not run on with bgp == NULL

This pattern is repeated in a number of places -- all need to be fixed. Perhaps a subroutine is a good idea?

yeah I thought about adding bgp into the parameters for bgp_vty_find_and_parse_afi_safi_vrf and then I realized that I wanted to come back through after we've fixed up more of the cli so I can make sure I have the pattern right.

pguibert6WIND · 2017-01-24T15:32:10Z

bgpd/bgp_route.c

+  if (!idx)
+    {
+      vty_out (vty, "View/Vrf Specified: %s is unknown", argv[5]->arg);
+      return CMD_WARNING;


can the number be set as an index value ? idx_val ?

This is probably wrong. I'll see what I can fix up.

louberger

A pedantic comment WRT vrf.h:
I think it would be best to

define VRF_UNKNOWN as VRF_MAX and VRF_MAX be some less than 2^16-1
that special values be defined above VRF_MAX
that the config interface prevent config of >= VRF_MAX

donaldsharp · 2017-01-25T14:23:56Z

@louberger To answer your question why MAX_INT -1? The VRF_UNKNOWN is MAX_INT so I couldn't use that value.

The use of this function was causing some confusion. As such let's add some documentation. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

When bgp_vty_find_and_parse_afi_safi_vrf detects a invalidly named vrf, it warns the user. There is no need for the calling function to warn again. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

NetDEF-CI · 2017-01-25T15:47:11Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-127/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

qlyoung · 2017-01-25T18:00:03Z

bgpd/bgp_route.c

+  if (argv_find(argv, argc, "community-list", &idx))
+    {
+      const char *clist_number_or_name = argv[++idx]->arg;
+      if (++idx < argc && strmatch (argv[idx]->arg, "exact-match"))


argv[idx]->text

pguibert6WIND · 2017-01-26T15:32:02Z

bgpd/bgp_vty.c

+ *
+ * The function returns the correct location in the parse tree for the
+ * last token found.
+ */


1 . returns 0 if error.

Fixed message.

donaldsharp · 2017-01-26T15:44:54Z

I will add this in a follow up commit, thanks for the reminder.

…

On Thu, Jan 26, 2017 at 10:32 AM, Philippe Guibert ***@***.*** > wrote: ***@***.**** commented on this pull request. ------------------------------ In bgpd/bgp_vty.c <#107 (review)> : > + * [<view|vrf> WORD] + * [<ipv4|ipv6> [<unicast|multicast|vpn|encap>]] + * The command parsing should still be ok. + * + * vty -> The vty for the command so we can output some useful data in + * the event of a parse error in the vrf. + * argv -> The command tokens + * argc -> How many command tokens we have + * idx -> The current place in the command, generally should be 0 for this function + * afi -> The parsed afi if it was included in the show command, returned here + * safi -> The parsed safi if it was included in the show command, returned here + * vrf -> The parsed vrf id if it was included in the show command, returned here + * + * The function returns the correct location in the parse tree for the + * last token found. + */ 1 . returns 0 if error. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#107 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKHDUNSN9oH5KQHa-lxrEX1KFhCTgMH7ks5rWLxzgaJpZM4Lr1CB> .

NetDEF-CI · 2017-01-26T15:59:54Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-137/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

NetDEF-CI · 2017-01-26T17:07:15Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-137/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

This fix addresses these things: 1) Clean up documentation as requested 2) Fix a wrong search for "exact-match" 3) Fix possible crash. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp · 2017-01-27T10:00:44Z

@louberger I fixed the crash you noticed that could happen.

NetDEF-CI · 2017-01-27T10:13:07Z

Continous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-140/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Failed

OmniOS amd64 build: Successful
CentOS6 amd64 build: Successful
NetBSD7 amd64 build: Successful
FreeBSD9 amd64 build: Successful
Ubuntu1204 amd64 build: Successful
FreeBSD10 amd64 build: Successful
Ubuntu1404 amd64 build: Successful
Debian8 amd64 build: Successful
FreeBSD11 amd64 build: Successful
OpenBSD60 amd64 build: Successful
NetBSD6 amd64 build: Successful
CentOS7 amd64 build: Successful
Fedora24 amd64 build: Successful

Ubuntu1604 amd64 build: Failed

Ubuntu1604 amd64 build: Unknown Log <log_configure.txt>
URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-140/artifact/CI014BUILD/ErrorLog/log_configure.txt
Ubuntu1604 amd64 build: Unknown Log <config.log>
URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-140/artifact/CI014BUILD/config.log/config.log
Ubuntu1604 amd64 build: No useful log found

NetDEF-CI · 2017-01-27T11:15:28Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-140/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Convert the idx to &idx to make our api more consistent Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

louberger · 2017-01-27T13:37:13Z

bgpd/bgp_vty.c

@@ -260,9 +260,12 @@ bgp_vty_find_and_parse_afi_safi_vrf (struct vty *vty, struct cmd_token **argv, i
  if (*vrf == VRF_UNKNOWN)
    {
      vty_out (vty, "View/Vrf specified is unknown: %s", vrf_name);
+      *idx = 0;


why set idx to null here? Isn't the return value of zero sufficient? For that matter, why do anything special here, the caller can check each of {afi, safi, vrf} as they see fit....

that is not setting it to null. I'm setting the value of what is pointed at to 0. Very different

sorry too many threads going on, was thinking about the next comment I was going to write on this (WRT bgp = NULL). the comment still applies, please read "why set *idx = 0 to here ..."

because it's the error case? We were unable to figure out what vrf was specified( the last line in the doc )

Add asserts to how I expect bgp_vty_find_and_parse_afi_safi to be used. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

NetDEF-CI · 2017-01-27T14:20:47Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-145/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

louberger · 2017-01-27T14:33:16Z

bgpd/bgp_vty.c

@@ -237,6 +237,10 @@ bgp_vty_find_and_parse_afi_safi_vrf (struct vty *vty, struct cmd_token **argv, i
 {
  char *vrf_name = NULL;

+  assert (afi);
+  assert (safi);
+  assert (*vrf == VRF_UNKNOWN);


don't you mean != VRF_UNKNOWN?

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

NetDEF-CI · 2017-01-27T15:38:35Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-146/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

NetDEF-CI · 2017-01-27T15:52:52Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-148/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Topotests misc improvements

donaldsharp added 10 commits January 23, 2017 22:34

lib: Add VRF_ALL define

ece35fd

Allow the specification of a VRF_ALL to be used for CLI. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Fix function used only within KEEP_OLD_VPN_COMMANDS

9dd6d53

The show_adj_route_vpn function is only currently used in conjunction with the KEEP_OLD_VPN_COMMANDS #define. Add this function to that define for the moment. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Remove unused function bgp_parse_safi

3289141

The bgp_parse_safi function is never called remove it. Especially as that later commits will properly handle what this function was trying to do. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Fix vpn commands cli

9fa4d33

The parser was incorrect for the 'set ... vpn nexthop ...' commands. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Cleanup 'show .... <bestpath|multipath>'

c41247f

Cleanup the bgp bestpath or multipath show commands. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgp: Fix 'show .... regexp REGEX...'

b00b230

Fix this command to use the correct format for a show command. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Cleanup 'view all' command

f2a8972

1) Make [<view|vrf> WORD] consistent 2) Fix inconsistent help string 3) Fix the show .. vrf all command Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Cleanup 'show .... statistics' command

e01ca20

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Clean up 'show bgp neighbor ...' commands

30a6a16

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp requested review from eqvinox and louberger January 24, 2017 12:37

bgpd: Fix crash with parsing the community

08435c2

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

louberger reviewed Jan 24, 2017

View reviewed changes

pguibert6WIND approved these changes Jan 25, 2017

View reviewed changes

louberger approved these changes Jan 25, 2017

View reviewed changes

louberger mentioned this pull request Jan 25, 2017

cleanup: VRF_UNKNOWN/VRF_ALL definition #117

Closed

donaldsharp added 2 commits January 25, 2017 09:37

bgpd: Add some documentation for bgp_vty_find_and_parse_afi_safi_vrf

7eeee51

The use of this function was causing some confusion. As such let's add some documentation. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Remove redundant warning message

c493f2d

When bgp_vty_find_and_parse_afi_safi_vrf detects a invalidly named vrf, it warns the user. There is no need for the calling function to warn again. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

qlyoung reviewed Jan 25, 2017

View reviewed changes

pguibert6WIND approved these changes Jan 26, 2017

View reviewed changes

pguibert6WIND reviewed Jan 26, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into bgpafisafi

7f12cb2

bgpd: Fix possible crash and some minor CR issues

0e37c25

This fix addresses these things: 1) Clean up documentation as requested 2) Fix a wrong search for "exact-match" 3) Fix possible crash. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

bgpd: Make usage of idx consistent

9317e17

Convert the idx to &idx to make our api more consistent Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

louberger mentioned this pull request Jan 27, 2017

Fix some encap/mpls commands #111

Merged

louberger reviewed Jan 27, 2017

View reviewed changes

bgp: Add Asserts and remove dead code

b4898a3

Add asserts to how I expect bgp_vty_find_and_parse_afi_safi to be used. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

louberger reviewed Jan 27, 2017

View reviewed changes

bgpd: Fix assert condition error.

69d143a

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

louberger merged commit aceb228 into FRRouting:master Jan 27, 2017

donaldsharp deleted the bgpafisafi branch November 30, 2017 14:46

cfra referenced this pull request in opensourcerouting/frr Nov 29, 2018

Merge pull request #107 from opensourcerouting/docker-fixes

7de07df

Topotests misc improvements

NetDEF-CI mentioned this pull request Mar 12, 2021

*: require ISO C11 + semicolons after file-scope macros #8121

Merged

Chromico mentioned this pull request Aug 21, 2022

Migrate test bgp set aspath replace from topotests opensourcerouting/frr#61

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bgpafisafi #107

Bgpafisafi #107

donaldsharp commented Jan 24, 2017

NetDEF-CI commented Jan 24, 2017

NetDEF-CI commented Jan 24, 2017

louberger left a comment

pguibert6WIND Jan 24, 2017

donaldsharp Jan 25, 2017

pguibert6WIND Jan 24, 2017

donaldsharp Jan 25, 2017

louberger Jan 26, 2017

louberger Jan 27, 2017

louberger Jan 27, 2017

donaldsharp Jan 27, 2017

pguibert6WIND Jan 24, 2017

donaldsharp Jan 25, 2017

louberger left a comment

donaldsharp commented Jan 25, 2017

NetDEF-CI commented Jan 25, 2017

qlyoung Jan 25, 2017

donaldsharp Jan 27, 2017

pguibert6WIND Jan 26, 2017

donaldsharp Jan 27, 2017

donaldsharp commented Jan 26, 2017 via email

NetDEF-CI commented Jan 26, 2017

NetDEF-CI commented Jan 26, 2017

donaldsharp commented Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

louberger Jan 27, 2017

donaldsharp Jan 27, 2017

louberger Jan 27, 2017

donaldsharp Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

louberger Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

Bgpafisafi #107

Bgpafisafi #107

Conversation

donaldsharp commented Jan 24, 2017

NetDEF-CI commented Jan 24, 2017

Continous Integration Result: SUCCESSFUL

NetDEF-CI commented Jan 24, 2017

Continous Integration Result: SUCCESSFUL

louberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louberger left a comment

Choose a reason for hiding this comment

donaldsharp commented Jan 25, 2017

NetDEF-CI commented Jan 25, 2017

Continous Integration Result: SUCCESSFUL

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldsharp commented Jan 26, 2017 via email

NetDEF-CI commented Jan 26, 2017

Continous Integration Result: SUCCESSFUL

NetDEF-CI commented Jan 26, 2017

Continous Integration Result: SUCCESSFUL

donaldsharp commented Jan 27, 2017

NetDEF-CI commented Jan 27, 2017

Continous Integration Result: FAILED

Get source and apply patch from patchwork: Successful

Building Stage: Failed

Ubuntu1604 amd64 build: Failed

NetDEF-CI commented Jan 27, 2017

Continous Integration Result: SUCCESSFUL

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetDEF-CI commented Jan 27, 2017

Continous Integration Result: SUCCESSFUL

Choose a reason for hiding this comment

NetDEF-CI commented Jan 27, 2017

Continous Integration Result: SUCCESSFUL

NetDEF-CI commented Jan 27, 2017

Continous Integration Result: SUCCESSFUL