[cmaster-next] [PATCH] bgpd: Fix crashes when no default bgp instance is configured.
Donald Sharp
sharpd at cumulusnetworks.com
Wed Dec 7 07:13:16 EST 2016
Bug imo. Hence the patch.
donald
On Tue, Dec 6, 2016 at 6:59 PM, Martin Winter
<mwinter at 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 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
>>>>>>
>
More information about the dev
mailing list