Skip to content

Fix Quagga/FRR parser on IPv6 BGP sessions#122

Merged
qiluo-msft merged 1 commit intosonic-net:masterfrom
qiluo-msft:qiluo/fixfrr
Jan 7, 2020
Merged

Fix Quagga/FRR parser on IPv6 BGP sessions#122
qiluo-msft merged 1 commit intosonic-net:masterfrom
qiluo-msft:qiluo/fixfrr

Conversation

@qiluo-msft
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft commented Jan 4, 2020

If there is no ipv4 or ipv6 BGP neighbors configured, the messages are a little different in different Quagga/FRR version.

Quagga in 201811

sonic> show ipv6 bgp su
No IPv6 neighbor is configured
sonic> 

FRR in 201811

sonic> show ip bgp ipv6 summary
sonic>

FRR in master

sonic> show ip bgp ipv6 summary
% No BGP neighbors found
sonic> 

Currently we could not handle 2nd or 3rd cases.

@qiluo-msft
Copy link
Copy Markdown
Contributor Author

@zhenggen-xu Could you please help review and test in your DUT?

l = ls[li]
if l.startswith('Neighbor '):
break
if l.startswith('No IPv'): # eg. No IPv6 neighbor is configured
Copy link
Copy Markdown
Collaborator

@zhenggen-xu zhenggen-xu Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define these strings as constant at beginning so it would be easier to maintain? #Resolved

Copy link
Copy Markdown
Contributor

@yxieca yxieca Jan 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea. We could define a list of criteria to skip (with comments) at one place and check them in a loop here. #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, both the strings and the functions such as startswith or endswith are important. So keep them together help maintenance.


In reply to: 363017378 [](ancestors = 363017378)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could define 2 lists of strings. one for starts with and one for ends with. They are still defined together. And the code to make use of these 2 lists are also together. This way, the data and handling are separate. I think it is cleaner this way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can improve this in future. This is a quick bug fix for 201811 FRR special use case required by @zhenggen-xu . Since it is tested by him, let's keep the code in this PR.

@zhenggen-xu
Copy link
Copy Markdown
Collaborator

@zhenggen-xu Could you please help review and test in your DUT?

Test worked fine with 201811 branch.

@qiluo-msft qiluo-msft requested review from 04diiguyi, lguohan and yxieca and removed request for 04diiguyi January 6, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants