[cmaster-next] [PATCH] bgpd: Fix crashes when no default bgp instance is configured.
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */ bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return; h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0; + bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p; bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop; if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family); + if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg; @@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default (); + if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */ h = bgp_default->rfapi; -- 2.7.4
Lou - This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts? Martin - The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey? donald On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
Thanks Donald, The VNC code really only works with default instance for now. I agree that changing this is the right thing. I think we need to hear from Paul Z on the debug topic. Lou On December 1, 2016 9:16:00 AM Donald Sharp <sharpd@cumulusnetworks.com> wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey?
donald
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
On December 1, 2016 9:16:00 AM Donald Sharp <sharpd@cumulusnetworks.com> wrote:
Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
I'm coming late to this discussion, sorry. Could you please elaborate on "unguarded"? cheers, ~!paul -- G. Paul Ziemba FreeBSD unix: 5:56PM up 102 days, 21:35, 7 users, load averages: 0.83, 0.92, 0.99
We've changed the output of the show ip bgp. To always display 'type=<route type>, subtype=..' it doubles the output length and additionally it probably breaks end user scripts. Looking at the called function, the extra information presented here really belongs in a detailed output instead of a 'show ip bgp' output. donald On Thu, Dec 1, 2016 at 8:54 PM, G. Paul Ziemba <pz-cmaster-next@ziemba.us> wrote:
On December 1, 2016 9:16:00 AM Donald Sharp <sharpd@cumulusnetworks.com> wrote:
Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
I'm coming late to this discussion, sorry. Could you please elaborate on "unguarded"?
cheers,
~!paul -- G. Paul Ziemba FreeBSD unix: 5:56PM up 102 days, 21:35, 7 users, load averages: 0.83, 0.92, 0.99
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
When removing the call into rfapi_vty_out_vncinfo if we are SAFI_UNICAST we move onto the next issue: E AssertionError: Routing Table verification failed for router r1, view 1: E *** actual BGP routing table E --- expected BGP routing table E *************** E *** 4,40 **** E Origin codes: i - IGP, e - EGP, ? - incomplete E E Network Next Hop Metric LocPrf Weight Path E ! 10.0.1.0/24 172.16.1.5 0 65005 i E ! 172.16.1.2 0 65002 i E ! 172.16.1.1 0 65001 i E ! 10.101.0.0/24 172.16.1.1 100 0 65001 i E ! 10.101.1.0/24 172.16.1.1 100 0 65001 i E ! 10.101.2.0/24 172.16.1.1 100 0 65001 i E ! 10.101.3.0/24 172.16.1.1 100 0 65001 i E ! 10.101.4.0/24 172.16.1.1 100 0 65001 i E ! 10.101.5.0/24 172.16.1.1 100 0 65001 i E ! 10.101.6.0/24 172.16.1.1 100 0 65001 i E ! 10.101.7.0/24 172.16.1.1 100 0 65001 i E ! 10.101.8.0/24 172.16.1.1 100 0 65001 i E ! 10.101.9.0/24 172.16.1.1 100 0 65001 i E ! 10.102.0.0/24 172.16.1.2 100 0 65002 i E ! 10.102.1.0/24 172.16.1.2 100 0 65002 i E ! 10.102.2.0/24 172.16.1.2 100 0 65002 i E ! 10.102.3.0/24 172.16.1.2 100 0 65002 i E ! 10.102.4.0/24 172.16.1.2 100 0 65002 i E ! 10.102.5.0/24 172.16.1.2 100 0 65002 i E ! 10.102.6.0/24 172.16.1.2 100 0 65002 i E ! 10.102.7.0/24 172.16.1.2 100 0 65002 i E ! 10.102.8.0/24 172.16.1.2 100 0 65002 i E ! 10.102.9.0/24 172.16.1.2 100 0 65002 i E ! 10.105.0.0/24 172.16.1.5 100 0 65005 i E ! 10.105.1.0/24 172.16.1.5 100 0 65005 i E ! 10.105.2.0/24 172.16.1.5 100 0 65005 i E ! 10.105.3.0/24 172.16.1.5 100 0 65005 i E ! 10.105.4.0/24 172.16.1.5 100 0 65005 i E ! 10.105.5.0/24 172.16.1.5 100 0 65005 i E ! 10.105.6.0/24 172.16.1.5 100 0 65005 i E ! 10.105.7.0/24 172.16.1.5 100 0 65005 i E ! 10.105.8.0/24 172.16.1.5 100 0 65005 i E ! 10.105.9.0/24 172.16.1.5 100 0 65005 i E ! 172.20.0.0/28 0.0.0.0 0 32768 i E --- 4,40 ---- E Origin codes: i - IGP, e - EGP, ? - incomplete E E Network Next Hop Metric LocPrf Weight Path E ! * 10.0.1.0/24 172.16.1.5 0 65005 i E ! * 172.16.1.2 0 65002 i E ! *> 172.16.1.1 0 65001 i E ! *> 10.101.0.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.1.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.2.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.3.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.4.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.5.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.6.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.7.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.8.0/24 172.16.1.1 100 0 65001 i E ! *> 10.101.9.0/24 172.16.1.1 100 0 65001 i E ! *> 10.102.0.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.1.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.2.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.3.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.4.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.5.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.6.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.7.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.8.0/24 172.16.1.2 100 0 65002 i E ! *> 10.102.9.0/24 172.16.1.2 100 0 65002 i E ! *> 10.105.0.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.1.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.2.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.3.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.4.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.5.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.6.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.7.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.8.0/24 172.16.1.5 100 0 65005 i E ! *> 10.105.9.0/24 172.16.1.5 100 0 65005 i E ! *> 172.20.0.0/28 0.0.0.0 0 32768 i E E assert 1 == 0 bgp_multiview_topo1/test_bgp_multiview_topo1.py:436: AssertionError =========================== 1 failed, 6 passed in 92.18 seconds =========================== I'm not sure now how to stop the test to allow me to connect to r1 to do a little debugging. Martin is this possible? donald On Thu, Dec 1, 2016 at 9:10 PM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
We've changed the output of the show ip bgp. To always display 'type=<route type>, subtype=..' it doubles the output length and additionally it probably breaks end user scripts.
Looking at the called function, the extra information presented here really belongs in a detailed output instead of a 'show ip bgp' output.
donald
On Thu, Dec 1, 2016 at 8:54 PM, G. Paul Ziemba <pz-cmaster-next@ziemba.us> wrote:
On December 1, 2016 9:16:00 AM Donald Sharp <sharpd@cumulusnetworks.com> wrote:
Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
I'm coming late to this discussion, sorry. Could you please elaborate on "unguarded"?
cheers,
~!paul -- G. Paul Ziemba FreeBSD unix: 5:56PM up 102 days, 21:35, 7 users, load averages: 0.83, 0.92, 0.99
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
On 1 Dec 2016, at 6:15, Donald Sharp wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey?
I brought up this question before. I can fix the test to ignore these extra lines, but I’m not sure this is the right way to do it. Maybe Lou (or someone else) can explain the need/use of this extra information and if this is intended or something forgotten from development and should be removed. (I worry about many other users doing screen scraping on this output) - Martin
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
Sigh -- been one of those days - Haven't had time to try to get Martin's tests running - Martin, What extra lines are you referring to? Lou On 12/1/2016 3:08 PM, Martin Winter wrote:
On 1 Dec 2016, at 6:15, Donald Sharp wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey? I brought up this question before. I can fix the test to ignore these extra lines, but I’m not sure this is the right way to do it.
Maybe Lou (or someone else) can explain the need/use of this extra information and if this is intended or something forgotten from development and should be removed. (I worry about many other users doing screen scraping on this output)
- Martin
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
On 1 Dec 2016, at 12:17, Lou Berger wrote:
Sigh -- been one of those days - Haven't had time to try to get Martin's tests running -
Martin, What extra lines are you referring to?
See https://ci1.netdef.org/browse/QUAGGA-CMASTERNEXT66-TOPOU1604-8 On a “show ip bgp” we get the extra line “type=bgp, subtype=0”, i.e.: 10.101.3.0/24 172.16.1.1 100 0 65001 i type=bgp, subtype=0 For me they look like forgotten debug info. (type “bgp” is something I assume on “show ip bgp” and subtype=0 means nothing to me) - Martin
On 12/1/2016 3:08 PM, Martin Winter wrote:
On 1 Dec 2016, at 6:15, Donald Sharp wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey? I brought up this question before. I can fix the test to ignore these extra lines, but I’m not sure this is the right way to do it.
Maybe Lou (or someone else) can explain the need/use of this extra information and if this is intended or something forgotten from development and should be removed. (I worry about many other users doing screen scraping on this output)
- Martin
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
I think I haven’t seen a reply to this. Feature or Bug? Is this extra line “type=bgp, subtype=0” in “show ip bgp” necessary or intentional or is this leftover debug code? If needed, can you explain the “subtype=0” and what that means? (And what other type then “type=bgp” is possible for a bgp route?) - Martin On 1 Dec 2016, at 15:02, Martin Winter wrote:
On 1 Dec 2016, at 12:17, Lou Berger wrote:
Sigh -- been one of those days - Haven't had time to try to get Martin's tests running -
Martin, What extra lines are you referring to?
See https://ci1.netdef.org/browse/QUAGGA-CMASTERNEXT66-TOPOU1604-8
On a “show ip bgp” we get the extra line “type=bgp, subtype=0”, i.e.:
10.101.3.0/24 172.16.1.1 100 0 65001 i type=bgp, subtype=0
For me they look like forgotten debug info. (type “bgp” is something I assume on “show ip bgp” and subtype=0 means nothing to me)
- Martin
On 12/1/2016 3:08 PM, Martin Winter wrote:
On 1 Dec 2016, at 6:15, Donald Sharp wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey? I brought up this question before. I can fix the test to ignore these extra lines, but I’m not sure this is the right way to do it.
Maybe Lou (or someone else) can explain the need/use of this extra information and if this is intended or something forgotten from development and should be removed. (I worry about many other users doing screen scraping on this output)
- Martin
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
Bug imo. Hence the patch. donald On Tue, Dec 6, 2016 at 6:59 PM, Martin Winter <mwinter@opensourcerouting.org> wrote:
I think I haven’t seen a reply to this.
Feature or Bug?
Is this extra line “type=bgp, subtype=0” in “show ip bgp” necessary or intentional or is this leftover debug code?
If needed, can you explain the “subtype=0” and what that means? (And what other type then “type=bgp” is possible for a bgp route?)
- Martin
On 1 Dec 2016, at 15:02, Martin Winter wrote:
On 1 Dec 2016, at 12:17, Lou Berger wrote:
Sigh -- been one of those days - Haven't had time to try to get Martin's tests running -
Martin, What extra lines are you referring to?
See https://ci1.netdef.org/browse/QUAGGA-CMASTERNEXT66-TOPOU1604-8
On a “show ip bgp” we get the extra line “type=bgp, subtype=0”, i.e.:
10.101.3.0/24 172.16.1.1 100 0 65001 i type=bgp, subtype=0
For me they look like forgotten debug info. (type “bgp” is something I assume on “show ip bgp” and subtype=0 means nothing to me)
- Martin
On 12/1/2016 3:08 PM, Martin Winter wrote:
On 1 Dec 2016, at 6:15, Donald Sharp wrote:
Lou -
This fixes the crashes that are found by the new topotest testing suite that Martin has created. As stated in the commit note, I have done no work to ensure that vnc is going to work correctly without a default bgp instance. We just no longer crash with these tests. Additionally I see lots of unguarded debugs in the vnc code. I believe these need to be cleaned up as well in some manner, thoughts?
Martin -
The bgp tests still fail, from what appears to be display output change. How would you like to proceed here? Send a patch to fix this issue to the topotests github doohickey?
I brought up this question before. I can fix the test to ignore these extra lines, but I’m not sure this is the right way to do it.
Maybe Lou (or someone else) can explain the need/use of this extra information and if this is intended or something forgotten from development and should be removed. (I worry about many other users doing screen scraping on this output)
- Martin
On Thu, Dec 1, 2016 at 9:11 AM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
Applied. If we need more to fix up VNC, it needs to go on top... On Thu, Dec 01, 2016 at 09:11:12AM -0500, Donald Sharp wrote:
The vnc code assumes that bgp must have a default instance. This code change checks to make sure that we do before proceeding. It makes no assurances that vnc will behave correctly without a default instance.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/rfapi/rfapi_import.c | 3 ++- bgpd/rfapi/vnc_import_bgp.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 8783024..77da4f9 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -4414,7 +4414,8 @@ rfapiProcessPeerDown (struct peer *peer) */
bgp = bgp_get_default (); /* assume 1 instance for now */ - assert (bgp); + if (!bgp) + return;
h = bgp->rfapi; assert (h); diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index 4215ce2..dc2640a 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -208,12 +208,17 @@ prefix_bag_free (void *pb) static void print_rhn_list (const char *tag1, const char *tag2) { - struct bgp *bgp = bgp_get_default (); - struct skiplist *sl = bgp->rfapi->resolve_nve_nexthop; + struct bgp *bgp; + struct skiplist *sl; struct skiplistnode *p; struct prefix_bag *pb; int count = 0;
+ bgp = bgp_get_default (); + if (!bgp) + return; + + sl = bgp->frapi->resolve_nve_nexthop; if (!sl) { zlog_debug ("%s: %s: RHN List is empty", (tag1 ? tag1 : ""), @@ -251,6 +256,8 @@ vnc_rhnck (char *tag) struct skiplistnode *p;
bgp = bgp_get_default (); + if (!bgp) + return; sl = bgp->rfapi->resolve_nve_nexthop;
if (!sl) @@ -1798,6 +1805,9 @@ vnc_import_bgp_exterior_add_route_it ( struct bgp *bgp_default = bgp_get_default (); afi_t afi = family2afi (prefix->family);
+ if (!bgp_default) + return; + h = bgp_default->rfapi; hc = bgp_default->rfapi_cfg;
@@ -1992,6 +2002,9 @@ vnc_import_bgp_exterior_del_route ( afi_t afi = family2afi (prefix->family); struct bgp *bgp_default = bgp_get_default ();
+ if (!bgp_default) + return; + memset (&pfx_orig_nexthop, 0, sizeof (struct prefix)); /* keep valgrind happy */
h = bgp_default->rfapi; -- 2.7.4
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
participants (5)
-
David Lamparter -
Donald Sharp -
G. Paul Ziemba -
Lou Berger -
Martin Winter