ospf6d: factor out lsdesc iterator

[acooks-at-bda]

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.

[aceelindem]

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.

[aceelindem]

Remove added whitespace.

[aceelindem]

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.

[rwgbsd]

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?

[acooks-at-bda]

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.

[aceelindem]

for

This is certainly needed since all the later OSPFv3 features (include SR and SRv6) are dependent on OSPFv3 Extended LSA support.

[vjardin]

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.

[rwgbsd]

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,
Realize that submitting a "pull request" does infact imply you believe this to be a mergable 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.

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:

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.

[acooks-at-bda]

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.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[acooks-at-bda]

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 struct ospf6_router_lsdesc or struct ospf6_network_lsdesc or struct ospf6_prefix.

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.