[cmaster-next] [PATCH] bgpd: Fix crashes when no default bgp instance is configured.

[Martin Winter]

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 at 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 at 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
>>>>>