-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ospf6d: factor out lsdesc iterator #15725
ospf6d: factor out lsdesc iterator #15725
Conversation
4f4e2ad
to
3ec5c18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to add code to factor out this iteration, why don't you do it for all the places rather than just ospf6_gr.c? There are iterations in ospf6_intra.c and possibly ospf_spf. c
The macros can go in ospf6_intra.h with the types.
@@ -452,25 +436,15 @@ static bool ospf6_gr_check_adjs_lsa_transit(struct ospf6_area *area, | |||
return true; | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove added whitespace.
ospf6d/ospf6_gr.c
Outdated
#define lsa_from_container(type, container) \ | ||
((struct type *)((char *)container->header + sizeof(struct ospf6_lsa_header))) | ||
|
||
#define lsdesc_start(lsdesc_type, lsa) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about prefixing these macro names with "lsa_link_"? For example, "lsa_link_lsdesc_start(). While the naming in ospf6d is not ideal, no sense in making it even more cryptic.
Is there some greater reason to change the code this way? Ie, is there some greater change coming that needs these iterators to be macrotized? Or is this just being done to compact the code and reduce repetition? |
I'm not suggesting this is the merge-able solution, but imo there is a worthwhile improvement to be made to the code that iterates over TLVs and LSAs. I'm working towards an implementation of RFC 8362 - OSPFv3 Link State Advertisement (LSA) Extensibility, which defines new LSAs in terms of TLV types, and will require iterators over TLVs in more places. That will be a significant functional change, which I may have to maintain as a patch for some time, so I'm trying to get early feedback on small patches. Thanks. |
This is certainly needed since all the later OSPFv3 features (include SR and SRv6) are dependent on OSPFv3 Extended LSA support. |
I believe the code clean up proposal that it suggested is a good idea. For sure, it'll be a major change for ospf6d, but it'll help to have a more mature code. @acooks-at-bda : could you explain your strategy for those changes ? For sure, it'll prevent doing some backports and maintenances but on the other side, we need to move ospf6d to become more mature. |
Thank you, that is what I was after, so this does fit into a grander plan, and with that information others have agreed that this is a worthwhile thing to do despite the issues it may raise with backporting. I am going to suggest to mark this pull request as WIP for now, and then proceed as @aceelindem aceelindem suggested:
|
3ec5c18
to
b2210a1
Compare
I've rebased this on the work in PR #16055, which unfortunately makes it hard to see what's new. Basically the iterators depend on the ospf6_lsa_end() function introduced in #16055 and if the order of the two PRs were reversed, there would still be a dependency between them. I am trying to control the amount of churn, while showing where I'm heading with this. Some churn just seems unavoidable. |
d881e60
to
7873b67
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Adds iterator macros for the descriptors in an LSA, to replace the number of repeated open coded pointer casts and pointer arithmetic. Signed-off-by: Andrew Cooks <acooks.at.bda@gmail.com>
Replace open coded iterators with new lsdesc iterator macro Signed-off-by: Andrew Cooks <acooks.at.bda@gmail.com>
7873b67
to
2499b53
Compare
Thanks for the comments and reviews. This PR was useful (to me) to learn about where and how the contents of LSAs are enumerated and think about a cleaner abstraction for accessing the 'records' in an LSA, whether they're It seems like this macro-based approach is not going to be extendable to work for TLVs in Extended LSAs, so I am closing this PR and will post a different proposal with that in mind. |
This work is superseded by PR #16199 |
Add a macro to iterate over the descriptors in an LSA and refactor the existing open coded iterators.
Other parts of ospf6d also iterate over
struct ospf6_*_lsdesc
and similar types, and perhaps a generic solution for all the instances can be found.