Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

route: add nexthop type #332

Closed
wants to merge 4 commits into from
Closed

route: add nexthop type #332

wants to merge 4 commits into from

Conversation

zstas
Copy link
Contributor

@zstas zstas commented Oct 13, 2022

The basic support for dumping nexthops and filling the cache.

@zstas zstas marked this pull request as ready for review October 17, 2022 11:05
@zstas
Copy link
Contributor Author

zstas commented Oct 17, 2022

Hello,
I'm new to libnl, so any feedback is much appreciated. Will be happy to address any comment.
Also, if you think that neighbor changes should be sent in a different PR - I can also do that.

Copy link
Owner

@thom311 thom311 left a comment

Choose a reason for hiding this comment

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

thank you for your patch. I am sorry for the long time it took me to reply. If you lost interest, I understand :( Anyway. Some comments...

include/linux-private/linux/nexthop.h Show resolved Hide resolved
libnl-cli-3.sym Show resolved Hide resolved
libnl-route-3.sym Show resolved Hide resolved
src/nl-nh-list.c Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
@zstas zstas force-pushed the main branch 2 times, most recently from b648fd2 to 221d0ba Compare July 27, 2023 10:44
@zstas
Copy link
Contributor Author

zstas commented Jul 27, 2023

Hi Thomas,

Thanks for the review. I tried to address all of them. Also I included another fix in the parser (in the previous patch only 2 group elements were allowed, I've increased it to 64). But unfortunately after rebasing github doesn't allow to compare the previous version with the current. But I've splitted the PR into 4 different commits (as you suggested), perhaps it makes the next review more easy.

I've been using the feature introduced in this patch for almost a year (for parsing fdb nexthops in linux), so I'd like to have this patch merged into upstream. So, I'm ready to improve this patch until you say it's good to go :)

include/linux-private/linux/nexthop.h Show resolved Hide resolved
include/netlink/route/neighbour.h Outdated Show resolved Hide resolved
include/netlink-private/types.h Show resolved Hide resolved
lib/route/neigh.c Outdated Show resolved Hide resolved
lib/route/neigh.c Outdated Show resolved Hide resolved
libnl-route-3.sym Show resolved Hide resolved
include/netlink-private/types.h Outdated Show resolved Hide resolved
lib/route/nh.c Show resolved Hide resolved
lib/route/nh.c Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
@zstas zstas force-pushed the main branch 4 times, most recently from af1c683 to 6bedab9 Compare July 28, 2023 07:14
@zstas
Copy link
Contributor Author

zstas commented Jul 28, 2023

I've addressed all the previous comments.
Also apologies for another last-minute change - since you mentioned nh_clone, I took another look and found that I wasn't copying nh_gateway and nh_group. Addressed that as well (in 3rd commit).

Copy link
Owner

@thom311 thom311 left a comment

Choose a reason for hiding this comment

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

Maybe you could take the patches from https://paste.centos.org/view/3c278476 (well, review them, and take the parts that you agree with).

It also requires you to rebase the branch again to latest main (because they use some new API from main branch).

lib/route/neigh.c Show resolved Hide resolved
lib/route/nh.c Outdated Show resolved Hide resolved
@zstas
Copy link
Contributor Author

zstas commented Jul 31, 2023

I intended to merge things from your patch today, but unfortunately the link is expired. Could you please share it one more time?

@thom311
Copy link
Owner

thom311 commented Jul 31, 2023

I didn't realize fpaste expires so fast. Sorry about that. Here again: https://paste.centos.org/view/bba2d453

I also just realized, that a change I did will cause a merge conflict when you rebase. Sorry about that. It should however be easy to resolved... Thanks.

@zstas
Copy link
Contributor Author

zstas commented Jul 31, 2023

I rebased, applied all your changes and also removed nh_master attribute from rtnl_nh struct (as it wasn't used anywhere). Could you please take a look once again?

thom311 added a commit that referenced this pull request Jul 31, 2023
@thom311
Copy link
Owner

thom311 commented Jul 31, 2023

merged as bc8573d.

Thank you for your efforts!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants